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

Investigate package-lock integrity issue #589

Closed
smaye81 opened this issue Oct 16, 2023 · 7 comments
Closed

Investigate package-lock integrity issue #589

smaye81 opened this issue Oct 16, 2023 · 7 comments

Comments

@smaye81
Copy link
Member

smaye81 commented Oct 16, 2023

For full context, see #526

The package-lock file seems to be missing resolved and integrity fields after installs in some instances. There is an open issue here with more context: npm/cli#6301

The recommendation is to delete node_modules and package-lock and rerun the install, which is a fragile process.

This PR is to track an investigation into whether we can implement a more permanent fix for this. Note the underlying cause is an issue with npm so we can't completely fix this, but we can at least harden our process to make sure these fields don't get removed.

@felschr
Copy link

felschr commented Oct 16, 2023

Some more popular npm issues about this: npm/cli#4263 npm/cli#4460

@felschr
Copy link

felschr commented Oct 16, 2023

After #590, packages with keys like packages/*/node_modules/* (most at the very bottom of the file) are still missing the resolved & integrity fields.
Seems to be another issue with npm. At least npm-lockfile-fix is able to fix those issues.

@smaye81
Copy link
Member Author

smaye81 commented Oct 16, 2023

After talking this over, I'm not sure what we can really do for this issue. It seems manual regeneration doesn't completely fix the problem based on the comment above. In addition we're a bit hesitant to add in the npm-lockfile-fix. I'd like to not introduce a Nix pkg or Python script into our process. This is ultimately a bug in npm and putting in any stopgap solution here doesn't seem worth the effort.

I definitely sympathize, though. I'm not very versed in Nix or its ecosystem. Can you help me to understand why the Nix pkg process needs those fields? From the linked issue, it seems like this affects caching somehow? I'm curious why other Nix pkgs don't have this same issue? For example, Prettier has a package listed, but the Prettier repo doesn't seem to have a package-lock at all. Is this because it falls back to the yarn.lock file for caching?

The Nix issue has a comment that seems to imply there's an escape hatch in case someone really wants to generate an empty cache, but the PR doesn't seem to mention it. Is there anyway to forego the caching at least until this npm issue is addressed?

@felschr
Copy link

felschr commented Oct 16, 2023

Nix builds packages in a declarative & fully reproducible way. That's why it needs to know the exact versions of all dependencies with their hashes.
The caching reference, you saw, due to Nix packages being built in isolation without network access.
Nixpkgs has some logic to download npm dependencies based on the lock file & then loads those dependencies into npm's cache and invokes npm cli in an offline mode.

There seems to be some work in nixpkgs to simplify working with incomplete/broken lock files, though:
NixOS/nixpkgs#261137 (comment)

@felschr
Copy link

felschr commented Oct 16, 2023

For example, Prettier has a package listed, but the Prettier repo doesn't seem to have a package-lock at all.

Prettier in nixpkgs is currently packaged in a different way that essentially downloads the prebuilt distributable from npm directly instead of doing a build from source.

So, for nixpkgs specifically, we can work around this issue by copying the package-lock.json, fixing it with npm-lockfile-fix & additional manual fixes if necessary. That's what I've now done in my PR: NixOS/nixpkgs#243432.
This just needs to be done on every package update. And while it can be partially automated, if the lockfile is broken (e.g. missing optional dependencies or version conflicts) it will require manual intervention.

@smaye81
Copy link
Member Author

smaye81 commented Oct 17, 2023

Yeah I understand. It's good that there's some movement in nixpkgs for helping with this issue. It's a shame that the lock file is so non-deterministic like that.

I am going to close this issue as there's nothing really for us to do here since this is an upstream bug. But if there's anything else on our side that could be addressed, let me know.

@smaye81 smaye81 closed this as completed Oct 17, 2023
@timostamm
Copy link
Member

#750 will avoid removing the hash fields during a version bump. It's not a guarantee that we'll always have them, but it should help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants