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

node-gyp: init at 10.2.0 #334377

Merged
merged 2 commits into from
Aug 16, 2024
Merged

node-gyp: init at 10.2.0 #334377

merged 2 commits into from
Aug 16, 2024

Conversation

dotlambda
Copy link
Member

Description of changes

see #229475

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.

@dotlambda
Copy link
Member Author

@ofborg eval

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this :). Changes look good to me, but I'd wait for more reviewers to respond... A nixpkgs-review report would be nice too.

@gepbird
Copy link
Contributor

gepbird commented Aug 15, 2024

Result of nixpkgs-review pr 334377 run on x86_64-linux 1

2 packages failed to build:
  • code-server
  • openvscode-server
6 packages built:
  • balena-cli
  • cdxgen
  • n8n
  • node-gyp
  • nodehun
  • vscode-js-debug

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-server fails on master too, no new failures 🚀

Tested the update script, changes LGTM!

Copy link
Contributor

@thomasjm thomasjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. The nodehun package works, so I'm happy as far as the packages I maintain.

I wish this didn't require adding 5,000 lines of package-lock.json though. One of the main motivations mentioned in #229475 is to avoid having a centralized update script that takes 4 hours to run. But breaking that up into separate packages that have their own update scripts and a large increase in lockfile data doesn't necessarily seem like an improvement. I see the node-gyp repo doesn't provide a lockfile but it would be nice if there were some other way to do this. Approving though because I have no idea what that way would be :)

@doronbehar
Copy link
Contributor

I wish this didn't require adding 5,000 lines of package-lock.json though.

I too feel the same. I was wondering, do we have a clue why upstream won't track such a file in their repository? That's the best I found:

nodejs/node-gyp@020e2bd (#1573)

@dotlambda
Copy link
Member Author

do we have a clue why upstream won't track such a file in their repository?

Technically projects that use node-gyp should include it in their devDependencies, so you shouldn't need to install it globally. I think such kinds of libraries are not recommended to ship a lock file.

@doronbehar doronbehar merged commit 24a76db into NixOS:master Aug 16, 2024
29 of 30 checks passed
@dotlambda dotlambda deleted the node-gyp branch August 16, 2024 16:00
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.

4 participants