Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] npm pack <git-dependency>#<commit-hash> fails in npm >= 9.6.5 #6723

Closed
2 tasks done
chmeliik opened this issue Aug 21, 2023 · 4 comments · Fixed by #7815
Closed
2 tasks done

[BUG] npm pack <git-dependency>#<commit-hash> fails in npm >= 9.6.5 #6723

chmeliik opened this issue Aug 21, 2023 · 4 comments · Fixed by #7815
Assignees
Labels
Bug thing that needs fixing Priority 2 secondary priority issue

Comments

@chmeliik
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

npm pack with a specific commit of a git dependency fails as follows:

$ npm pack vercel/ms#1304f150b38027e0818cc122106b5c7322d68d0c                                   
npm ERR! GitFetcher requires an Arborist constructor to pack a tarball

npm ERR! A complete log of this run can be found in: /home/acmiel/.npm/_logs/2023-08-21T08_27_53_861Z-debug-0.log

Expected Behavior

Expected npm pack to succeed (like it does in version <= 9.6.4).

Note that it still works for things that are not commit hashes:

$ npm pack vercel/ms#3.0.0-canary.1
...
ms-3.0.0-canary.1.tgz

Steps To Reproduce

  1. With npm version 9.6.5 or higher
  2. Run npm pack vercel/ms#1304f150b38027e0818cc122106b5c7322d68d0c or any other git dependency + commit hash
  3. And get npm ERR! GitFetcher requires an Arborist constructor to pack a tarball

Environment

  • npm: 9.8.1
  • Node.js: 18.17.1
  • OS Name: Fedora 37
  • System Model Name: Lenovo ThinkPad X1 Carbon
  • npm config:
; node bin location = /usr/bin/node-18
; node version = v18.17.1
; npm local prefix = /tmp/test
; npm version = 9.8.1
; cwd = /tmp/test
; HOME = /home/acmiel
; Run `npm config ls -l` to show all defaults.
@chmeliik chmeliik added Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release labels Aug 21, 2023
chmeliik added a commit to chmeliik/cachito that referenced this issue Aug 21, 2023
npm>=9.6.5 has a bug which causes `npm pack` to fail for git
dependencies: npm/cli#6723

The latest `nodejs-npm` available in the Fedora 38 repos is 9.6.7.
Downgrade to the only older version still available, which is 9.5.0.

Signed-off-by: Adam Cmiel <[email protected]>
chmeliik added a commit to chmeliik/cachito that referenced this issue Aug 21, 2023
npm>=9.6.5 has a bug which causes `npm pack` to fail for git
dependencies: npm/cli#6723

The latest `nodejs-npm` available in the Fedora 38 repos is 9.6.7.
Downgrade to the only older version still available, which is 9.5.0.

Signed-off-by: Adam Cmiel <[email protected]>
github-merge-queue bot pushed a commit to containerbuildsystem/cachito that referenced this issue Aug 22, 2023
npm>=9.6.5 has a bug which causes `npm pack` to fail for git
dependencies: npm/cli#6723

The latest `nodejs-npm` available in the Fedora 38 repos is 9.6.7.
Downgrade to the only older version still available, which is 9.5.0.

Signed-off-by: Adam Cmiel <[email protected]>
@chmeliik
Copy link
Author

So, I think I've found the problem.

npm pack calls pacote.manifest() without passing in an Arborist (why would it, it's just getting a package.json)

const manifest = await pacote.manifest(spec, this.npm.flatOptions)

For a git dependency which is "hosted" and resolved, GitFetcher.manifest uses FileFetcher.manifest instead

return this.spec.hosted && this.resolved
? FileFetcher.prototype.manifest.apply(this)

FileFetcher.manifest calls extract()

return cacache.tmp.withTmp(this.cache, this.opts, dir =>
this.extract(dir)

extract calls _tarballFromResolved

streamHandler(this[_istream](this[_tarballFromResolved]()))

And finally, GitFetcher's _tarballFromResolved does a full clone, prepare and would go on to do pretty much an entire pack operation if not for the missing Arborist error

this[_clone](dir => this[_prepareDir](dir)
.then(() => new Promise((res, rej) => {
if (!this.Arborist) {
throw new Error('GitFetcher requires an Arborist constructor to pack a tarball')
}

@chmeliik
Copy link
Author

chmeliik commented Sep 12, 2023

I think the fix is to remove the FileFetcher.manifest usage and always just clone

       return Promise.resolve(this.package)
     }
 
-    return this.spec.hosted && this.resolved
-      ? FileFetcher.prototype.manifest.apply(this)
-      : this[_clone](dir =>
+    return this[_clone](dir =>
         this[_readPackageJson](dir + '/package.json')
           .then(mani => this.package = {
             ...mani,

Would anyone happen to know why FileFetcher.manifest was used in the first place?

chmeliik added a commit to chmeliik/pacote that referenced this issue Sep 13, 2023
To get the manifest for a hosted, resolved git package, pacote uses
FileFetcher.manifest. FileFetcher.manifest calls extract(), which calls
_tarballFromResolved.

This means getting the manifest involves nearly a complete pack
operation, including 'npm install', the 'prepare' script etc. It also
means getting the manifest requires an Arborist constructor (causes
npm/cli#6723).

Remove the special handling of hosted, resolved git packages, handle all
git packages the same way: clone the repo (or get the tarball from a
hosted git server) and read the package.json.

The effects for hosted, resolved git packages:

- getting the manifest no longer involves packing the tarball
- getting the manifest no longer requires an Arborist constructor
- the manifest no longer includes an _integrity (same as un-hosted or
  un-resolved git packages)

Signed-off-by: Adam Cmiel <[email protected]>
chmeliik added a commit to chmeliik/pacote that referenced this issue Sep 13, 2023
To get the manifest for a hosted, resolved git package, pacote uses
FileFetcher.manifest. FileFetcher.manifest calls extract(), which calls
_tarballFromResolved.

This means getting the manifest involves nearly a complete pack
operation, including 'npm install', the 'prepare' script etc. It also
means getting the manifest requires an Arborist constructor (causes
npm/cli#6723).

Remove the special handling of hosted, resolved git packages, handle all
git packages the same way: clone the repo (or get the tarball from a
hosted git server) and read the package.json.

The effects for hosted, resolved git packages:

- getting the manifest no longer involves packing the tarball
- getting the manifest no longer requires an Arborist constructor
- the manifest no longer includes an _integrity (same as un-hosted or
  un-resolved git packages)

Signed-off-by: Adam Cmiel <[email protected]>
@itavero
Copy link

itavero commented May 3, 2024

I'm getting the same error when trying to use npm exec / npx with a commit hash on npm 10.7.0.
Should this already have been fixed (as I do see some referenced issues thath ave been closed)?

@milaninfy
Copy link
Contributor

evidently it works for short commit hash format and fails for long one.

~/workarea/npm-cli $ npx npm/cli#ec105f400281a5bfd17885de1ea3d54d0c231b27 -v
npm error GitFetcher requires an Arborist constructor to pack a tarball
npm error A complete log of this run can be found in: /Users/milaninfy/.npm/_logs/2024-08-30T17_55_32_028Z-debug-0.log

~/workarea/npm-cli $ npx npm/cli#ec105f4 -v                                 
Need to install the following packages:
github:npm/cli#ec105f4
Ok to proceed? (y) y

10.8.3

@milaninfy milaninfy added Priority 2 secondary priority issue and removed Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release labels Aug 30, 2024
@milaninfy milaninfy self-assigned this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 2 secondary priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants