Support for node >15 and npm>7 attempt #3 #166
Conversation
|
My direction with #165 was to support building a project with lockfile v1 and compare results for nodejs 14, 16 and 18. I have no control over the package lock definition. Do you suggest that I make a preprocessor to convert v1 to v2 and then use npmlock2nix, I'm not sure which direction to go. I do use a linked .npmrc to customize the build, note the If it helps I could wrap up a test using one of our project package-lock.json. Also, If it's a matter of still supporting v1 API, could we not just reintroduce the the node_modules/.hooks script and preInstallLinks? Then ignore them on nodejs >15.
I also wondered about nix garbage collection with packaged tar gz. Thanks for pointing out the $out/build-support mechanic. |
| nativeBuildInputs = [ jq openssl ]; | ||
|
|
||
| propagatedBuildInputs = [ | ||
| nodejs |
There was a problem hiding this comment.
I used coreutil to patch binaries to avoid having to rebuild packages when changing nodejs version. It worked but might not be the right solution for this.
|
|
||
| # Description: Takes a parsed lockfile and returns the patched version as an attribute set | ||
| # Type: { sourceHashFunc :: Fn } -> parsedLockedFile :: Set -> { result :: Set, integrityUpdates :: List { path, file } } | ||
| patchLockfile = sourceOptions: content: |
There was a problem hiding this comment.
Some node modules seem to provides package.json with dependencies with "*" version in v2 lockfile. I managed to work around this issue by finding the latest package version to replace those. I will try to find a repro case.
There was a problem hiding this comment.
Do you mean the packages.$name.resolved field end up with a "*" value? 😱 If that's the case, I did not hit that one yet. I'm not even sure we can do something here, I'd consider that as a npm bug we should fix upstream.
If you're talking about the packages.$name.dependencies.$name, I'd argue it's not an issue for us. We exclusively use the resolved field to remotely download packages in the v2 codebase.
There was a problem hiding this comment.
Finally the issue is when package.json have "latest" as version specs. Not sure if this is a "dependencies" only issue. ex. https://github.com/compodoc/live-server/blob/master/package.json
internal-v2.nix
Outdated
| patched = patchLockfile sourceOptions parsedLockFile; | ||
| replacement = writeText "packages_hashes.sed" (lib.concatMapStrings | ||
| (dep: lib.optionalString (dep ? id) '' | ||
| s @${dep.id}@ ${lib.removeSuffix "\n" (builtins.readFile dep.hash)} g; |
There was a problem hiding this comment.
Is this what you mean by IFD? we build dep in order to produce our package-lock.json?
But if dep can be cached is this still considered an IFD?
Ah, okay, I was confused about that part. It makes sense now :)
Are you automating the generation of the If you write it manually, I think the best approach would be to use the If you're generating that code, I guess we could either go the pre-processor1 route to determine what is kind of lockfile you're looking at. Alternatively, we also could perform this pre-processor phase at the top-level of
If this project breaks the current codebase, it'd be great if we could to add it to the test!
The preInstallLinks are still supported in the v1 codebase. They are also ignored in the v2 codebase (ie. nodejs >15). The sourceOverrides mechanism is supported for both codebases. I first thought about removing it for the v1 codebase, but I later realized they are already part of the Footnotes
|
|
my npmlock2nix code is not autogenerated. By preprocessor I meant doing a npm install outside nix to get a lockfile v2. to use npmlock2nix.v2. Because with current v1 (master) I cannot build a v1 lockfile with npm 7 or nodejs >15.
Sorry I said v1 when I was referencing #165. That's the only thing I removed, since it was supported by the API update I made. |
picnoir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
3 times, most recently
from
22efe39 to
330f8d5
Compare
Right! I get it now. I did not think about that case! I assume this particular case will only happen during this v1 -> v2 transition phase we're currently in. As far as I understand, npm coming from nodejs > 15 will override the v1 lockfile with a v2 as soon as you'll run Provided nodejs 14 EOL is planned on the 2023/04/30, I think we're unlikely to see much lockfiles v1 for the still maintained projects past that date. Dropping the v1 support for the v2 lockfiles simplifies the codebase quite a lot, reducing the potential bugs surface and easing the maintenance on the long run. I think the preprocessor approach would be preferable here. That being said, you might not be the only one falling into that use case. It'd be probably a good idea to create a
Aha! Indeed, good point, I've been overlooking that. There's no reason for only supporting the |
Yes, I saw exactly that when running Thanks to both of you. This is a massive improvement, and I'm looking forward to seeing this merged :-) |
This simplifies the photoprism build, and is using nix-community/npmlock2nix#166.
|
@NinjaTrappeur sorry, this is way too much. I don't have the time to go through all of this - and the PS: kudos for the very clean code, good documentation and tests |
|
I think part of this is due to the current duplication, as the code for v1 and v2 is kept separately (as per @andir 's suggestion). Some of the diff noise might go away by first diffing the old code against the v1 parts in here, and then diffing the v1 implementation with the v2 one. Another alternative could be to drop v1 support immediately, and ask users to use an old version of npmlock2nix if they still want to package stuff where they can't create a v2 lockfile. WDYT? |
|
@flokli to be clear, the v1/v2 separation isn’t necessarily a bad idea.. the complexity is what is horrifying to me. And that of course is nobody’s fault. It’s the result of having to deal with something that isn’t meant as a public interface 😅 i think @andir ’s reasoning is sound when he wants to separate things to avoid too much separate case handling and what not. @flokli as for your suggestions, hm.. i don’t know from the top of my head what the npm release schedule is. Maybe there should be a clear goal/statement on what npmlock2nix strives to support and we should base that to consider what options we have. What do you think @andir ? |
|
By the way, i certainly won’t be stopping progress on this just because i can’t review a big PR. The code per se looks clean and we have good documentation and tests as per usual in this project. I assume @andir and others are more likely to be digging through details. I’d just involve myself in talking about broader stuff like what we want to support when ;) |
|
I'll to review this tomorrow. |
I'm trying to be realistic here: My vision right now is that the |
andir
left a comment
andir
left a comment
There was a problem hiding this comment.
First things first:
Thank you, @NinjaTrappeur and also @tlxzilia for the work on this! We need v2 support :)
As discussed earlier, I've proposed splitting the API versions (v1/v2) so we can leave the old v1 code untouched in the repository to be used with older packages that aren't ever updated to a newer release.
I've not started reviewing everything that is within this PR but rather tried using it with v2 lockfiles that are currently working with the v1 code. I think we must at least show that this approach and code works for large numbers of projects that are using the v2 format before merging it and potentially attracting users (that then are unhappy because it doesn't work for them).
The first project I tried was zigbee2mqtt with the following expression (in the npmlock2nix directory):
let
pkgs = import ./nix { };
npmlock2nix = import ./default.nix { inherit pkgs; };
in
npmlock2nix.v2.build rec {
src = pkgs.fetchFromGitHub {
owner = "Koenkk";
repo = "zigbee2mqtt";
rev = "08ceae235e34a1f4a13f183e835a1b52fd2ac674";
sha256 = "08dfvj4b78dc65mzsbpybb13y7vv882pv8mn5q211a2pc03dxy43";
};
node_modules_attrs = {
packageLockJson = src + "/package-lock.json";
nativeBuildInputs = [ pkgs.python3 pkgs.nukeReferences ];
postBuild = ''
find . -type f -iname "package.json" -exec nuke-refs {} \;
'';
};
buildCommands = [
"npm run build"
];
installPhase = ''
mkdir -p $out/lib/node_modules
cp -r . $out/lib/node_modules/zigbee2mqtt
mkdir -p $out/bin
find $out/lib/node_modules -type f -iname "package.json" -delete
cat - <<EOF > $out/bin/zigbee2mqtt
#!/usr/bin/env sh
export NODE_PATH="${placeholder "out"}/lib/node_modules/zigbee2mqtt:${placeholder "out"}/lib/node_modules/zigbee2mqtt/node_modules"
set -ex
cd ${placeholder "out"}/lib/node_modules/zigbee2mqtt
exec ./run.js
EOF
cat - <<EOF > $out/lib/node_modules/zigbee2mqtt/run.js
#!/usr/bin/env node --trace-warnings
require('./index.js')
EOF
chmod +x $out/bin/zigbee2mqtt
chmod +x $out/lib/node_modules/zigbee2mqtt/run.js
# zigbee2mqtt now checks the configured engine version during runtime
cp package.json $out/lib/node_modules/zigbee2mqtt/
'';
}The code above worked when using the v1 attribute and when not specifying an attribute. The expected warning message was produced. After switching to the v2 attribute the build fails as NPM thinks one of the dependency tarballs might be corrupt:
these derivations will be built:
/nix/store/xb9zpilw11hiqx117nv832ahvbzxxkhj-package-lock.json.drv
/nix/store/9mgdpdskr88yad69lsv5xrphqjkcci9j-zigbee2mqtt-1.28.0-dev.drv
/nix/store/hcfss9y58l4yhnglvzqff7hr13bxr2a7-zigbee2mqtt-1.28.0-dev.drv
building '/nix/store/xb9zpilw11hiqx117nv832ahvbzxxkhj-package-lock.json.drv'...
building '/nix/store/9mgdpdskr88yad69lsv5xrphqjkcci9j-zigbee2mqtt-1.28.0-dev.drv'...
patching sources
configuring
no configure script, doing nothing
building
npm WARN tarball tarball data for @rjsf/bootstrap-5@file:/nix/store/42nahl90h9ylcc4jh1kc0k1ywk8qh3hm-rjsf-bootstrap-5-4.2.0.tgz (sha512-gHwtGSeteSl3LiSOk+rIENiVjI7yaMTYcxqroXZxErstz/5WcZV5Wme+8XCYBB7yLhMiWPvNlDS9Nr4urADIdQ==) seems to be corrupted. Trying again.
npm WARN tarball tarball data for @rjsf/bootstrap-5@file:/nix/store/42nahl90h9ylcc4jh1kc0k1ywk8qh3hm-rjsf-bootstrap-5-4.2.0.tgz (sha512-gHwtGSeteSl3LiSOk+rIENiVjI7yaMTYcxqroXZxErstz/5WcZV5Wme+8XCYBB7yLhMiWPvNlDS9Nr4urADIdQ==) seems to be corrupted. Trying again.
npm ERR! code ENOENT0m reifyNode:node_modules/core-jsKjsmoot
npm ERR! syscall open
npm ERR! path /nix/store/42nahl90h9ylcc4jh1kc0k1ywk8qh3hm-rjsf-bootstrap-5-4.2.0.tgz
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, open '/nix/store/42nahl90h9ylcc4jh1kc0k1ywk8qh3hm-rjsf-bootstrap-5-4.2.0.tgz'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent
npm ERR! A complete log of this run can be found in:
npm ERR! /build/.npm/_logs/2022-10-24T12_34_30_856Z-debug.log
builder for '/nix/store/9mgdpdskr88yad69lsv5xrphqjkcci9j-zigbee2mqtt-1.28.0-dev.drv' failed with exit code 254
cannot build derivation '/nix/store/hcfss9y58l4yhnglvzqff7hr13bxr2a7-zigbee2mqtt-1.28.0-dev.drv': 1 dependencies couldn't be built
error: build of '/nix/store/hcfss9y58l4yhnglvzqff7hr13bxr2a7-zigbee2mqtt-1.28.0-dev.drv' failed
This is a good proxy for a user upgrading to a newer version of npmlock2nix and following our adivce in the warning that was produced.
The second project It tried was cinny with the following expression:
let
pkgs = import ./nix { };
npmlock2nix = import ./default.nix { inherit pkgs; };
in
npmlock2nix.v1.build rec {
src = pkgs.fetchFromGitHub {
owner = "ajbura";
repo = "cinny";
rev = "91a6916f4ca1b876028280856cba8ce12115a81e";
sha256 = "05hlllc05nqn6j3bb59488gficqffss1l6bn88456iv4kiwcg6sm";
};
installPhase = [ "cp -r dist $out" ];
node_modules_attrs = {
buildInputs = [ pkgs.vips ];
nativeBuildInputs = [ pkgs.pkgconfig pkgs.python3 ];
};
preBuild = ''
sed -e "/'process.env': JSON.stringify(process.env),/d" -i webpack.common.js
'';
}And it just worked 🥳 🎺 🎊 .
So the first project (zigbee2mqtt) shows some kind of weird error where we seem to be interpreting the lockfiles different from what NPM thinks about them?
Is any of you able to dig into this?
Right now, my criterias to merging this in are mostly support for projects we used to support, the new v2 support as far as we can test it and general test coverage for the features must exist.
There is some code duplication right now, but I'd rather do that in a follow-up PR that refactors code than in this PR that already is too large for a sensible review anyway.
|
I had a look at this. One thing about the v1/v2 splitting is that there is a lot of common code which I think can and should be shared. This was just a quick attempt. I think the functions around GitRev/url/* parsing and validation also seemed similar and could perhaps be made reusable? Not entirely sure. There is some hash function that could also be moved but i ran out of time and would have had to adjust the tests as well. TL;DR i would like to advocate for extracting reusable code as seen in my attempt. @andir reasonable to you? I know you wanted the v1/v2 split. |
|
Yeah, I want the code to be refactored by I'd not do it as part of this PR. Lets get this working with proper tests first and then refactor the duplicate code. |
|
The problem here seems to be
I assume our other github logic detection gets confused about this, and we match github URLs too eagerly in |
|
Hey Andi,
It's definitely on my todolist. I have been off for better part of last
month, I just came back last week and had to deal with a month-worth of
professional "urgent" matter.
The todo list is unpiling slowly, pushing through this PR is the thing
coming after what I'm busy-with at the moment.
You can expect some movement during this week.
|
|
The issue you faced with zigbee2mqtt turned out being a consequence of Npm trying to interpret query strings for Fixed in I tried building the |
picnoir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
from
673e11a to
2d4c67f
Compare
|
Removed a useless file as pointed out in 68196dd#r87717991 |
The idea is to keep two different codebases in the same repository. A "legacy" codebase in charge of parsing the v1 npm lockfiles, a new codebase in charge of parsing the v2 lockfiles. The user can then select which codebase to use through the v1 and v2 top-level attributesets. This approach should simplify the migration once we'll decide to rollout the v1 lockfiles support. In the meantime, it allow us to add the lockfile v2 support without changing too much the API for the v1 users. In case the user directly calls npmlock2nix instead of selecting a v1 or v2 namespace, we issue a warning and proceed to use the v1 namespace to keep backward compatibility. In practice, internal-v1.nix has been extracted from the current master HEAD (5c4f247) internal.nix. internal-v2.nix has been extracted from tlxzilla's PR (165). The v1 tests have also been extracted from master. These tests will be migrated to the v2 codebase in a subsequent commit. The test-v1 unit and integration tests have been slightly modified to use the fully qualified npmlock2nix.v1 prefix instead of the unqualified npmlock2nix one. I know: the diff of this commit looks scary. That being said, it's only moving some already existing components around. It sadly cannot be minimized further.
Current status: Build and integration tests working \0/ Source tests working \0/ Node-Modules tests broken /0\ BUT I FINALLY HAVE A PLAN TO FIX THE ISSUE!!!!! Current issue: Unless we perform a sourceOverride to the npm package, its tarball is directly downloaded from the npm registry without being unpacked and patched by nix. There's an issue with this approach: the shebangs. We end up having some unpatched node shebangs failing on runtime. I think the solution to this is to recursively walk the unpacked node_modules tree and patch one by one the executable files.
Fixes the node-modules-attributes-are-passed-through unit test for the lockfiles v2 implementation. The directories only containing symlinked are getting deleted during the tarball unpack process. In that particular test case, we symlink cwebp to `./vendor/cwebp`. `./vendor` gets deleted after unpackaging, meaning the build fails when trying to symlink nixpkgs's cwebp to `./vendor/cwebp`: we need to re-create the vendor directory first.
Some package dependencies can be expressed using a http link to a
tarball. Such as:
{
"author": "",
"dependencies": {
"@matrix-org/olm": "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.4.tgz"
},
"description": "",
"devDependencies": {},
"license": "ISC",
"main": "index.js",
"name": "url-as-version",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"version": "1.0.0"
}
The version and other metadata attributes will be extracted by npm and
written in the package-lock.json file. For instance, for the previous
example:
{
"dependencies": {},
"lockfileVersion": 2,
"name": "url-as-version",
"packages": {
"": {
"dependencies": {
"@matrix-org/olm": "3.2.4"
},
"license": "ISC",
"name": "url-as-version",
"version": "1.0.0"
},
"node_modules/@matrix-org/olm": {
"integrity": "sha512-ddaXWILlm1U0Z9qpcZffJjBFZRpz/GxQ1n/Qth3xKvYRUbniuPOgftNTDaxkEC4h04uJG5Ls/OdI1YJUyfuRzQ==",
"license": "Apache-2.0",
"resolved": "file:///nix/store/qn1b7cpsw383kprpzvq4r1x3yis9bczn-olm-3.2.4.tgz",
"version": "3.2.4"
}
},
"requires": true,
"version": "1.0.0"
}
The issue however, is that npm will invalidate the package-lock file
for each call and try to re-fetch the remote tarball to re-generate
it: there's no integrity mechanism in package.json, the remote URL
could have changed.
We go around that issue by replacing the URL in the version field of
package.json by a wildcard. Exactly like what we already do for github
dependencies.
Npm extended the forms a github url can take in the lockfiles v2 format. Because of this changes, we were unable to run the testPatchLockfileTurnsGitHubUlsIntoStorePaths test from the patch-lockfile test suite. On top of the github:org/repo#revision format we were already supporting for the lockfiles v1, we can now also parse urls in the form of git@github.com/owner/repo.git#revision and git+ssh://git@github.com/owner/repo.git#revision. Adapting the parseGitHubRef function to support those. Adding some unit tests in the parse-github-ref test suite to make sure we parse everything correctly.
The npm lockfiles v2 are meant to be retro-compatible with the "old"
v1 format. This retrocompatibility is implemented using using 2
top-level attributes: packages, the attribute containing the
v2-specific packages descriptions. dependencies, the attribute
containing the legacy packages descriptions.
{
"packages": {
"": { ... },
"node_modules/left-pad": { ... }
},
dependencies: {
...
},
"version": ...
...
}
It turns out that npm won't complain if we drop entirely the
"dependencies" section as long as the description contained in the
"packages" attribute is complete-enough to build the node_modules
folder.
It's a good very news for us: it allows us to drop entirely support
for the v1 lockfile format in the v2 codebase. That's quite a massive
win on the long run: the machinery required to parse the v2 lockfile
is **much** simpler from the one required for the legacy format.
In this commit, we drop altogether all the support needed to parse the
lockfiles v1.
---------------------------
Loosely related: it turns out that npm disables the integrity check
when "null" is specified as an integrity. This behaviour is similar to
what we used to do except we don't have to calculate the store path
integrity anymore. It makes things a little bit easier code-wise.
We might have to revise this assumption if upstream end up changing
its behaviour in that regard.
Let's please nixpkgs-fmt.
Some npm dependencies are bundled, meaning their node_modules directory is contained as part of their parent tgz distribution. Like a regular package, a bundled package can contain some executable scripts. That being said, these exec scripts won't be listed in the `bin` section of their parent's package-lock.json file. This is to be expected, those are transitive dependencies, they're not meant to be listed there. When patching a package through a source_override, we look whether or not a node_modules directory is present. If it's there, it means we potentially have some unpatched shebangs in these transitive dependencies. We recurse in them using "patchShebangs".
Relaxing dependencies version bounds for github refs. If we happen to leave them as is, it'll force NPM to checkout the remote repo to get the actual ref. At this point, we already pinned everything through the "resolved" field, there's no point in keeping the unresolved reference in dependencies. We can relax those to "*".
Add some instructions about the new v1/v2 prefixes.
Some bundled dependencies can end up not having a "resolved" nor "integrity" field. That's the case for the yallist dependency from the vega-embed project. In that cases, we don't want to patch anything or throw an error, but instead we'll leave the lockfile element as it is in the patched lockfile. Adding a example-project having a bundled dependency without a resolved field to test this behavior.
As per https://github.com/npm/npm/blob/2e3776bf5676bc24fec6239a3420f377fe98acde/doc/files/package.json.md#peerdependencies, peerDependencies are only used to express compatibility. However, it seems npm still does network access to fetch some information about some of them, which fails in the sandbox. This network access happen when some peer-dependencies are not available through the node-modules spec, ie. their build receipe are not available in the lockfile. We don't have enough informations to fetch those: we lack both the resolved link and their integrity. These dependencies do not influence the node_modules/ directory in any ways. npmlock2nix only purpose is to build this directory, meaning we can safely ignore the peer-dependencies during the node_modules/ nix build. This can be observed in @storybook/react, which contains require-from-string@"^2.0.2" as peerDependency only. In the lockfile, require-from-string isn't mentioned as a dependency, yet npm still tries to look up information: npm WARN ERESOLVE overriding peer dependency0m Completed in 0ms npm WARN While resolving: @storybook/react@6.5.10 npm WARN Found: require-from-string@undefined npm WARN node_modules/require-from-string npm WARN npm WARN Could not resolve dependency: npm WARN peer require-from-string@"^2.0.2" from @storybook/react@6.5.10 npm WARN node_modules/@storybook/react npm WARN dev @storybook/react@"^6.5.9" from the root project Adding a unit test checking we indeed remove the peerDependencies from the original lockfile.
Npm gets confused when a lockfile file:// resolve field contains a
question mark in its name. It interprets that as a query string and
strips it away.
IE.
For a dependency like:
{
"dep": {
"version": "4.2.0",
"resolved": "file:///nix/store/hash-blahblah.tgz?querystring=true",
"integrity": "sha512-gHwtGSeteSl3LiSOk+rIENiVjI7yaMTYcxqroXZxErstz/5WcZV5Wme+8XCYBB7yLhMiWPvNlDS9Nr4urADIdQ==",
},
}
Npm will try to open hash-blahblah.tgz instead of the actual file
hash-blahblah.tgz?querystring=true.
We go around this issue by making sure we remove anything that could
be interpreted as a query string by Npm in the store name.
picnoir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
from
2d4c67f to
3283c59
Compare
This tells users that they are using the unversioned API. This is fine but they should migrate to the versioned API soon. This also adds a warning for the v2 users as their API might still change and we should be upfront about that.
tl;dr: we want simple Nix code. No fancy advanced features if they can be avoided
andir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
from
ffdc767 to
aff1c4c
Compare
andir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
2 times, most recently
from
e3774ca to
02e0514
Compare
andir
force-pushed
the
v2-lockfile-backwardcompatible-with-tests
branch
from
02e0514 to
7b2db50
Compare
|
I'll merge this in as is now. CI is passing. The documentation is a bit of a mess. I've tried do bring back the v1 documentation where still relevant. We will have to do a few more passes through the docs and bug fixing before the |
6 participants
This PR is built on top of #165. Massive kudos to @tlxzilia for the head start.
Originally, I planned to:
internal-v1.nixcodebase and its tests. Credit where it's due, this was not my idea: this approach has been suggested by @andir <3example-projects.Things diverged from this plan along the way:
dependenciestop-level attr. This simplifies a great deal the v2 codebase, since we can drop all the v1-specific machinery from it. This has been implemented in 68196ddYou can find more details about what I've done in the commit messages.
There's two more non-critical points to address, but at this point, the PR is already pretty massive, I'm already afraid it could be too big to be reviewed/merged. I'd like to work on those more incrementally once this gets merged for those. Namely:
$out/build-support.Integrityfield of the patched npm dependencies: I set those tonull. If it is set tonull, it seems like npm won't try to compute the dependency integrity. It makes things simpler for us: we don't need to perform an IFD to compute the hashes anymore. That being said, it could backfire at any point if npm decides at some point to change its mind about this undocumented behavior.