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] Lost support submodules #2774

Closed
trofim24 opened this issue Feb 25, 2021 · 20 comments
Closed

[BUG] Lost support submodules #2774

trofim24 opened this issue Feb 25, 2021 · 20 comments
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release Release 8.x work is associated with a specific npm 8 release Release 9.x work is associated with a specific npm 9 release Release 10.x

Comments

@trofim24
Copy link

Current Behavior:

npm version 7 (I tried both 7.0.0 and the last 7.5.6) does not download submodules.

Expected Behavior:

npm version 6.14.11 has support submodules.

Steps To Reproduce:

I created 3 repos to reproduce the problem. The first contains package.json with dependencies to install. The second contains submodule. The third contains only Readme.md

  1. git clone https://github.com/trofim24/npm.git
  2. npm install
    If you do this on version 6.14.11 then the node_modules/npm-bug/npm-bug-submodule folder will exist.
    If you do this on version 7.5.6 then the node_modules/npm-bug/npm-bug-submodule folder will not exist.

Environment:

  • OS: Windows 10 2004 or Debian GNU/Linux 9.13
  • npm: 6.14.11 and npm: 7.5.6
@trofim24 trofim24 added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Feb 25, 2021
@trofim24
Copy link
Author

Any updates?

@zpv
Copy link

zpv commented Mar 26, 2021

Also experiencing this issue and breaks my workflow. I reverted back to 6 in the meantime.
Related (old) issue: npm/npm#6700

@darcyclarke darcyclarke added Priority 2 secondary priority issue and removed Needs Triage needs review for next steps labels Apr 9, 2021
@darcyclarke darcyclarke added Priority 1 high priority issue and removed Priority 2 secondary priority issue labels Mar 28, 2022
cztomsik added a commit to cztomsik/graffiti that referenced this issue Oct 15, 2022
npm workaround - it's documented but it doesn't work
@trofim24
Copy link
Author

Any updates?

@ljharb ljharb added Release 8.x work is associated with a specific npm 8 release Release 9.x work is associated with a specific npm 9 release labels Feb 13, 2023
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Feb 28, 2023
`npm install` used to `git clone --recursive` the things that it builds out of GitHub; but it [doesn't do that anymore](npm/cli#2774). Unfortunately, we depend on that feature, because a submodule is how https://github.com/Level/rocksdb pulls in the Facebook code.

This fixes a really absconse error message, which is a huge pain to debug BTW:

```
npm ERR! make: *** No rule to make target 'Release/obj.target/rocksdb/deps/rocksdb/rocksdb/cache/cache.o', needed by 'Release/obj.target/deps/rocksdb/rocksdb.a'.  Stop.
```

(That's because there's no source code under `deps/rocksdb/rocksdb`, on account of that directory being a Git submodule.)

It seems... difficult to convince npm to keep its temporary files for inspection. I had to temporarily push a hacked `package.json` into the `master` branch of `epfl-si/rocksdb-node`, with an `install` script entry that reads

```
    "install": "node-gyp-build || (sleep 3600; exit 1)",
```

Confusingly enough, modern `npm`s will do the right thing (i.e. pull the submodules) when one runs e.g. `npm i -g epfl-si/rocksdb-node`; but not when said `epfl-si/rocksdb-node` is a dependency in a `package.json`, as is the case here. Go figure.
@domq
Copy link

domq commented Feb 28, 2023

Curiously enough, using the latest version of npm, (e.g.) npm i -g https://github.com/Level/rocksdb does what we want i.e. pull the deps/rocksdb/rocksdb submodule out of Facebook. But not when "github.com/Level/rocksdb" is in a package.json as a dependency.

@ljharb
Copy link
Contributor

ljharb commented Feb 28, 2023

@domq presumably npm install https://github.com/Level/rocksdb would match package.json (global installs are different than local ones)

@domq
Copy link

domq commented Feb 28, 2023

@ljharb I'm not sure what you mean with “match package.json”, in my experiments it's the git behavior (i.e. whether the clone is recursive or not), that differs between the two situations.

@ljharb
Copy link
Contributor

ljharb commented Feb 28, 2023

@domq sorry, what i mean is, you said npm install -g does what you want, but npm install (when it's in package.json) does not. I was asking about what npm install https://github.com/Level/rocksdb does, which is a third, potentially different thing.

@domq
Copy link

domq commented Feb 28, 2023

@ljharb oh, I get it now, sorry. The behavior of npm install https://github.com/Level/rocksdb is indeed the same as when the dependency is in a package.json as you guessed, i.e. a nonrecursive clone is used for the build.

domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Feb 28, 2023
`npm install` used to `git clone --recursive` the things that it builds out of GitHub; but it [doesn't do that anymore](npm/cli#2774). Unfortunately, we depend on that feature, because a submodule is how https://github.com/Level/rocksdb pulls in the Facebook code.

This fixes a really absconse error message, which is a huge pain to debug BTW:

```
npm ERR! make: *** No rule to make target 'Release/obj.target/rocksdb/deps/rocksdb/rocksdb/cache/cache.o', needed by 'Release/obj.target/deps/rocksdb/rocksdb.a'.  Stop.
```

(That's because there's no source code under `deps/rocksdb/rocksdb`, on account of that directory being a Git submodule.)

It seems... difficult to convince npm to keep its temporary files for inspection. I had to temporarily push a hacked `package.json` into the `master` branch of `epfl-si/rocksdb-node`, with an `install` script entry that reads

```
    "install": "node-gyp-build || (sleep 3600; exit 1)",
```

Confusingly enough, modern `npm`s will do the right thing (i.e. pull the submodules) when one runs e.g. `npm i -g epfl-si/rocksdb-node`; but not when said `epfl-si/rocksdb-node` is a dependency in a `package.json`, as is the case here. Go figure.
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Mar 1, 2023
`npm install` used to `git clone --recursive` the things that it builds out of GitHub; but it [doesn't do that anymore](npm/cli#2774). Unfortunately, we depend on that feature, because a submodule is how https://github.com/Level/rocksdb pulls in the Facebook code.

This fixes a really absconse error message, which is a huge pain to debug BTW:

```
npm ERR! make: *** No rule to make target 'Release/obj.target/rocksdb/deps/rocksdb/rocksdb/cache/cache.o', needed by 'Release/obj.target/deps/rocksdb/rocksdb.a'.  Stop.
```

(That's because there's no source code under `deps/rocksdb/rocksdb`, on account of that directory being a Git submodule.)

It seems... difficult to convince npm to keep its temporary files for inspection. I had to temporarily push a hacked `package.json` into the `master` branch of `epfl-si/rocksdb-node`, with an `install` script entry that reads

```
    "install": "node-gyp-build || (sleep 3600; exit 1)",
```

Confusingly enough, modern `npm`s will do the right thing (i.e. pull the submodules) when one runs e.g. `npm i -g epfl-si/rocksdb-node`; but not when said `epfl-si/rocksdb-node` is a dependency in a `package.json`, as is the case here. Go figure.
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Mar 1, 2023
💡 Because of npm/cli#2774 , this means that we must now use version 6 of npm to rebuild (!), like so:

```
npx npm@6 install
``
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Mar 1, 2023
`npm install` used to `git clone --recursive` the things that it builds out of GitHub; but it [doesn't do that anymore](npm/cli#2774). Unfortunately, we depend on that feature, because a submodule is how https://github.com/Level/rocksdb pulls in the Facebook code.

This fixes a really absconse error message, which is a huge pain to debug BTW:

```
npm ERR! make: *** No rule to make target 'Release/obj.target/rocksdb/deps/rocksdb/rocksdb/cache/cache.o', needed by 'Release/obj.target/deps/rocksdb/rocksdb.a'.  Stop.
```

(That's because there's no source code under `deps/rocksdb/rocksdb`, on account of that directory being a Git submodule.)

It seems... difficult to convince npm to keep its temporary files for inspection. I had to temporarily push a hacked `package.json` into the `master` branch of `epfl-si/rocksdb-node`, with an `install` script entry that reads

```
    "install": "node-gyp-build || (sleep 3600; exit 1)",
```

Confusingly enough, modern `npm`s will do the right thing (i.e. pull the submodules) when one runs e.g. `npm i -g epfl-si/rocksdb-node`; but not when said `epfl-si/rocksdb-node` is a dependency in a `package.json`, as is the case here. Go figure.
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Mar 1, 2023
💡 Because of npm/cli#2774 , this means that we must now use version 6 of npm to rebuild (!), like so:

```
npx npm@6 install
``
domq pushed a commit to epfl-si/zeebe-db-monitor that referenced this issue Mar 1, 2023
`npm install` used to `git clone --recursive` the things that it builds out of GitHub; but it [doesn't do that anymore](npm/cli#2774). Unfortunately, we depend on that feature, because a submodule is how https://github.com/Level/rocksdb pulls in the Facebook code.

This fixes a really absconse error message, which is a huge pain to debug BTW:

```
npm ERR! make: *** No rule to make target 'Release/obj.target/rocksdb/deps/rocksdb/rocksdb/cache/cache.o', needed by 'Release/obj.target/deps/rocksdb/rocksdb.a'.  Stop.
```

(That's because there's no source code under `deps/rocksdb/rocksdb`, on account of that directory being a Git submodule.)

It seems... difficult to convince npm to keep its temporary files for inspection. I had to temporarily push a hacked `package.json` into the `master` branch of `epfl-si/rocksdb-node`, with an `install` script entry that reads

```
    "install": "node-gyp-build || (sleep 3600; exit 1)",
```

Confusingly enough, modern `npm`s will do the right thing (i.e. pull the submodules) when one runs e.g. `npm i -g epfl-si/rocksdb-node`; but not when said `epfl-si/rocksdb-node` is a dependency in a `package.json`, as is the case here. Go figure.
@cjbj
Copy link

cjbj commented May 25, 2023

Still not fixed in npm 9.6.7

@DeeDeeG
Copy link

DeeDeeG commented Oct 24, 2023

This is both a bug and a documentation issue. I can confirm this bug (the loss of submodule/recursive cloning in dependencies) is still true in npm 10.

The documentation claims submodules will be cloned, but submodules are actually not cloned anymore.

If the repository makes use of submodules, those submodules will be cloned as well.

- From: https://docs.npmjs.com/cli/v10/commands/npm-install

Test Results:

  • Submodules are cloned (as in, it works as described in the npm install documentation) if you use npm 6 (npm v6.14.18). ✅
  • Submodule cloning doesn't happen for (as in, docs are inaccurate for) npm 7 (v7.24.2), npm 8 (v8.19.4), npm 9 (v9.9.0), and npm latest/10 (v10.2.1). ❌

Implications of the bug:
This loss of submodule/recursive cloning breaks some packages when used as git dependencies. For example, Atom's superstring library doesn't build properly on Windows with npm 7.x or newer, as a git dependency, due to a missing vendored win-iconv build-time dependency, which is vendored in as a submodule under the superstring repo. This was working as of npm 6.

What to do about it?:
I'd love for submodule/recursive cloning for git dependencies to come back in npm latest. It would un-break some dependencies we still rely on. But if this support is not coming back any time soon, I feel like the docs should be updated in the mean-time to not claim submodules will be cloned when they actually won't. Thanks for considering.

@ljharb
Copy link
Contributor

ljharb commented Jan 18, 2024

fwiw, I suddenly have this need as well on two different repos - i'm using a submodule to pull in test fixtures, and having npm install ensure the submodules are available would be very helpful.

@reggi
Copy link
Contributor

reggi commented Sep 16, 2024

I added to the docs that this is just a limitation of using git packages directly here in this npm/documentation#1279 available here https://docs.npmjs.com/about-packages-and-modules

@reggi reggi closed this as completed Sep 16, 2024
@ljharb
Copy link
Contributor

ljharb commented Sep 16, 2024

@reggi is there no plan to add this support? (even by some kind of dev-only postinstall hook)

@reggi
Copy link
Contributor

reggi commented Sep 17, 2024

@ljharb there is no plan to add this support. If someone in userland wants to make a postinstall hook that's perfectly fine! 😸

@ljharb
Copy link
Contributor

ljharb commented Sep 17, 2024

How would I do such a thing in userland, such that it won't affect my package's consumers?

@reggi
Copy link
Contributor

reggi commented Sep 17, 2024

idk @ljharb nevermind, idk what you meant by dev-only postinstall hook

@ljharb
Copy link
Contributor

ljharb commented Sep 17, 2024

@reggi i mean that i'd need a hook that runs when i run npm install in a foo project, but doesn't run when someone does npm install foo. I'm not aware of any such hook, which means this can't be done in userland, but I could be wrong.

@reggi
Copy link
Contributor

reggi commented Sep 17, 2024

@ljharb gotcha thanks for the clarification I'm not aware of such a hook either.

@ljharb
Copy link
Contributor

ljharb commented Sep 17, 2024

then can this be reopened, since the use case isn't currently achievable?

@threema-danilo
Copy link

@reggi the documentation at https://docs.npmjs.com/cli/v10/commands/npm-install still claims that submodules are cloned:

screenshot-20241021-114613

Unfortunately the missing support for submodules makes it impossible to install certain libraries through git, for example node-argon2 which includes the argon2 library from upstream as a submodule.

Please reconsider adding support for submodules. If you decide against it, please update the documentation to remove any claims of such support.

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 1 high priority issue Release 7.x work is associated with a specific npm 7 release Release 8.x work is associated with a specific npm 8 release Release 9.x work is associated with a specific npm 9 release Release 10.x
Projects
None yet
Development

No branches or pull requests