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

npmHooks.npmInstallHook: resolve symlinks when copying node_modules #333759

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

felschr
Copy link
Member

@felschr felschr commented Aug 10, 2024

Description of changes

Sometimes node_modules can contain symlinks that are currently being copied as is and can cause broken symlinks in the output. Symlinks are very common in projects using npm workspaces to reference other projects within the workspace.

I recently packaged protoc-gen-es but it currently fails with a runtime error where it complains about missing npm dependencies: #243432 (comment)
The issue can be resolved with the changes in this PR.

The issue for protoc-gen-es specifically could also be fixed in a postInstall script:

  postInstall = ''
    rm -rf $out/lib/node_modules/protobuf-es/node_modules/@bufbuild
    cp -rL node_modules/@bufbuild $out/lib/node_modules/protobuf-es/node_modules/
  '';

But to me it seems that this is a general issue that should ideally be solved in the npm install hook itself.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@felschr felschr requested a review from mweinelt as a code owner August 10, 2024 19:54
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Aug 10, 2024
@felschr felschr changed the base branch from staging to master August 10, 2024 19:54
@felschr felschr removed the request for review from mweinelt August 10, 2024 19:54
@github-actions github-actions bot removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Aug 10, 2024
@felschr
Copy link
Member Author

felschr commented Aug 10, 2024

Initially I thought this would classify as a mass rebuild but I just noticed it "only" affects 414 packages & previous changes to npm hooks also targeted master. So, I just switched the target branch.

felschr added a commit that referenced this pull request Sep 9, 2024
This workaround won't be needed anymore once #333759 is merged, which
fixes the node_modules copying behavior in npm-install-hooks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants