fix: npm deps on git+https urls #2592
Conversation
|
jbedard
force-pushed
the
test-http-git
branch
2 times, most recently
from
484f51b to
9820b5b
Compare
jbedard
force-pushed
the
test-http-git
branch
from
9820b5b to
94c1da5
Compare
jbedard
changed the title
npm/private/utils.bzl
Outdated
|
|
||
| def _is_git_repository_url(url): | ||
| return url.startswith("git+ssh://") or url.startswith("git+https://") or url.startswith("git@") | ||
| return url.startswith("git+ssh://") or url.startswith("git+https://") or url.startswith("git@") or url.startswith("https://git@") |
There was a problem hiding this comment.
The pnpm <9 (lockfile v54, 60, 61) tests reproduce this where the lockfile itself has the git repo specified as:
resolution: {repo: https://git@github.com/jquery/jquery.git}
I assume this is possible for pnpm 9+ as well but 9+ must fallback to non-git tarball https resolution instead of git.
There was a problem hiding this comment.
See newer comment regarding deleting this is_git_repository_url
…2593) While debugging an error in #2592 it was failing to output an error message instead saying: ``` expected string for sequence element 0, got '@@aspect_bazel_lib~~toolchains~bsd_tar_darwin_arm64//:tar' of type Label ``` This change makes it output the args in a format such as: ``` Failed to extract package tarball. '[Label("@@aspect_bazel_lib~~toolchains~bsd_tar_darwin_arm64//:tar"), "-xf", "package.tgz", "--strip-components", "1", "-C", "package", "--no-same-owner", "--no-same-permissions"]' exited with 1: ``` which might have the ugly `[...]` braces but at least it's not failing to output the error message. ### Changes are visible to end-users: no ### Test plan - Covered by existing test cases - Manual testing
…2593) While debugging an error in #2592 it was failing to output an error message instead saying: ``` expected string for sequence element 0, got '@@aspect_bazel_lib~~toolchains~bsd_tar_darwin_arm64//:tar' of type Label ``` This change makes it output the args in a format such as: ``` Failed to extract package tarball. '[Label("@@aspect_bazel_lib~~toolchains~bsd_tar_darwin_arm64//:tar"), "-xf", "package.tgz", "--strip-components", "1", "-C", "package", "--no-same-owner", "--no-same-permissions"]' exited with 1: ``` which might have the ugly `[...]` braces but at least it's not failing to output the error message. ### Changes are visible to end-users: no ### Test plan - Covered by existing test cases - Manual testing
jbedard
force-pushed
the
test-http-git
branch
3 times, most recently
from
cfce535 to
a06e6f6
Compare
| ) | ||
|
|
||
| def _is_git_repository_url(url): | ||
| return url.startswith("git+ssh://") or url.startswith("git+https://") or url.startswith("git@") |
There was a problem hiding this comment.
There is also https://git@ which I had originally added to this. But there is also plain https:// which might be a git repo (and it probably ends with .git but might end with .git#commithash).
I think it's better to not do things based on URL and instead based on if a commit attr is specified, which we required anyway (see removed fail()).
|
|
||
| reproducible = False | ||
| package_src = _EXTRACT_TO_DIRNAME | ||
| if utils.is_git_repository_url(rctx.attr.url): |
There was a problem hiding this comment.
We explicitly set url to the lockfile repo field which for git+https version specifiers will be the plain https://github.com/foo/bar.git URL and not appear to be a git protocol.
We then assert that when fetching from git a commit is required. So instead lets just do things based on the commit attribute...
| "@isaacs/cliui": "8.0.2", | ||
| "debug": "ngokevin/debug#9742c5f383a6f8046241920156236ade8ec30d53", | ||
| "esbuild": "0.27.0", | ||
| "highlightjs-git-https-notar": "git+https://gitea.osmocom.org/vyanitskiy/highlight.js.git#58dc5961f6f2bb8bc8bb1e7ce39f268a2fdd874f", |
There was a problem hiding this comment.
Github seems to provide tarballs for all commits so the pnpm 9+ lockfile references the tarball instead of using the git protocol.
Using this non-github public git repo ensures the git protocol is used on all pnpm version
| engines: {node: '>= 0.4'} | ||
|
|
||
| highlight.js@git+https://gitea.osmocom.org/vyanitskiy/highlight.js.git#58dc5961f6f2bb8bc8bb1e7ce39f268a2fdd874f: | ||
| resolution: {commit: 58dc5961f6f2bb8bc8bb1e7ce39f268a2fdd874f, repo: https://gitea.osmocom.org/vyanitskiy/highlight.js.git, type: git} |
There was a problem hiding this comment.
Notice the type: git where all the jquery ones are tarball
jbedard
force-pushed
the
test-http-git
branch
from
a06e6f6 to
3d403ac
Compare
Found while adding more tests for #2403 - Covered by existing test cases - New test cases added
3 participants
Found while adding more tests for #2403
Changes are visible to end-users: no
Test plan