Fix fetching npm packages from git repositories #2403
Conversation
|
|
|
|
Can you add a test which fails without your fix? |
I will do that. Currently I am waiting for my company to approve the CLA. As soon as this has been done, I will continue on this PR. |
thomasdraebing
force-pushed
the
fix-git-fetch
branch
from
d87d286 to
aa91d9f
Compare
|
@jbedard I finally got the approval to sign the CLA and added an E2E test. If you just check out the first commit that contains only the test, it fails, and it succeeds with the second commit containing the fix. |
|
Would it be possible to test it by adding a package to |
|
Otherwise I think you need to add the e2e test to the list in |
Aspect_rules_js supports fetching a git repository containing an npm package. However, to follow that path, it is required that the URL of the repository is prefixed with `git+`. However, this URL is then also configured as the remote for the git repository. The `git+` prefix is not supported by git and thus fetching fails with: git: 'remote-git+https' ist kein Git-Befehl. Siehe 'git --help'. To prevent this, the `git+`-prefix is now being removed, if it is present before setting it as remote url.
thomasdraebing
force-pushed
the
fix-git-fetch
branch
from
aa91d9f to
5d6cb83
Compare
Done
I have updated the test. |
|
@thomasdraebing I'm just looking to port this to the 3.x branch and also add more testing and I keep getting errors when trying to install this Is there something special with this |
|
Even if I just @thomasdraebing you must have some local config making this work work? |
Aspect_rules_js supports fetching a git repository containing an npm
package. However, to follow that path, it is required that the URL of
the repository is prefixed with `git+`. However, this URL is then also
configured as the remote for the git repository. The `git+` prefix is
not supported by git and thus fetching fails with:
git: 'remote-git+https' ist kein Git-Befehl. Siehe 'git --help'.
To prevent this, the `git+`-prefix is now being removed, if it is
present before setting it as remote url.formation ends up in the git
log.
---
### Changes are visible to end-users: no
### Test plan
- Manual testing; please provide instructions so we can reproduce:
Install an NPM package from a git repository. The pnpm-lock.yaml may
look like this:
```
lockfileVersion: '6.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
dependencies:
highlight.js:
specifier: ^11.11.1
version: 11.11.1
highlightjs-ttcn3:
specifier: https://gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d
version: gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d
packages:
/[email protected]:
resolution: {integrity: sha512-Xwwo44whKBVCYoliBQwaPvtd/2tYFkRQtXDWj1nackaV2JPXx3L0+Jvd8/qCJ2p+ML0/XVkJ2q+Mr+UVdpJK5w==}
engines: {node: '>=12.0.0'}
dev: false
gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d:
resolution:
type: git
repo: git+https://gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git
commit: 6daccff309fca1e7561a43984d42fa4f829ce06d
name: highlightjs-ttcn3
version: 0.0.1
dependencies:
highlight.js: 11.11.1
dev: false
```
|
@jbedard just using pnpm or npm also fails for me for that repository. Actually for the same reason, it tries to download a tar-archive, which isn't available on that git server. It works however using yarn from which we are in the progress of migrating from due to the removal of support of |
|
How did you generate the pnpm-lock in your new e2e test then? |
Found while adding more tests for #2403 ### Changes are visible to end-users: no ### Test plan - Covered by existing test cases - New test cases added
Found while adding more tests for #2403 - Covered by existing test cases - New test cases added
Aspect_rules_js supports fetching a git repository containing an npm
package. However, to follow that path, it is required that the URL of
the repository is prefixed with `git+`. However, this URL is then also
configured as the remote for the git repository. The `git+` prefix is
not supported by git and thus fetching fails with:
git: 'remote-git+https' ist kein Git-Befehl. Siehe 'git --help'.
To prevent this, the `git+`-prefix is now being removed, if it is
present before setting it as remote url.formation ends up in the git
log.
---
- Manual testing; please provide instructions so we can reproduce:
Install an NPM package from a git repository. The pnpm-lock.yaml may
look like this:
```
lockfileVersion: '6.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
dependencies:
highlight.js:
specifier: ^11.11.1
version: 11.11.1
highlightjs-ttcn3:
specifier: https://gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d
version: gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d
packages:
/[email protected]:
resolution: {integrity: sha512-Xwwo44whKBVCYoliBQwaPvtd/2tYFkRQtXDWj1nackaV2JPXx3L0+Jvd8/qCJ2p+ML0/XVkJ2q+Mr+UVdpJK5w==}
engines: {node: '>=12.0.0'}
dev: false
gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git#6daccff309fca1e7561a43984d42fa4f829ce06d:
resolution:
type: git
repo: git+https://gitea.osmocom.org/ttcn3/highlightjs-ttcn3.git
commit: 6daccff309fca1e7561a43984d42fa4f829ce06d
name: highlightjs-ttcn3
version: 0.0.1
dependencies:
highlight.js: 11.11.1
dev: false
```
|
The case used in the e2e test is actually the case, I wanted to solve for the Gerrit project. There I added the dependency manually to the lock-file based on the previously existing yarn.lock file. I guess, the same bug should be fixed in pnpm as well. |
|
You manually created the lockfile? Does that mean it might be invalid and your fix is for a scenario that pnpm would never reproduce? |
|
I don't believe that it is invalid. The resolution format is implemented here: https://github.com/pnpm/pnpm/blob/v8.15.9/resolving/git-resolver/src/index.ts#L36. This works: However, pnpm again uses this data to compute the tar-ball download URL. This is not the case for rules_js. It will actually try to clone. Just see the diff of my fix and that is clearly broken. |
|
If it is impossible to generate that lockfile via pnpm then I would not consider that to be a valid lockfile. See e758431 where I was trying to add tests for this. Anywhere I use How did you end up with |
3 participants
Aspect_rules_js supports fetching a git repository containing an npm package. However, to follow that path, it is required that the URL of the repository is prefixed with
git+. However, this URL is then also configured as the remote for the git repository. Thegit+prefix is not supported by git and thus fetching fails with:git: 'remote-git+https' ist kein Git-Befehl. Siehe 'git --help'.
To prevent this, the
git+-prefix is now being removed, if it is present before setting it as remote url.formation ends up in the git log.-->
Changes are visible to end-users: no
Test plan
Install an NPM package from a git repository. The pnpm-lock.yaml may look like this: