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

yarnInstallHook: init #328544

Merged
merged 5 commits into from
Sep 4, 2024
Merged

yarnInstallHook: init #328544

merged 5 commits into from
Sep 4, 2024

Conversation

lelgenio
Copy link
Contributor

@lelgenio lelgenio commented Jul 19, 2024

Description of changes

When attempt to use npmInstallHook on a yarn project using fetchYarnDeps, npm frequently tries to connect to the internet during the prune step.

Turns out replacing npm prune with yarn install --production=true ... resolves this issue.

ping @doronbehar

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.

@lelgenio
Copy link
Contributor Author

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

5 packages built:
  • blade-formatter
  • codefresh
  • element-call
  • postlight-parser
  • yarnInstallHook

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.

Looks good, thanks for working on this!

@lelgenio lelgenio marked this pull request as ready for review August 14, 2024 04:46
@winterqt
Copy link
Member

I have no spoons for this, sorry.

@philiptaron philiptaron removed their request for review August 17, 2024 15:10
@lelgenio lelgenio force-pushed the yarn-install-hook branch 2 times, most recently from 3e5e44c to 0f8dff5 Compare August 17, 2024 19:42
@lelgenio
Copy link
Contributor Author

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

5 packages built:
  • blade-formatter
  • codefresh
  • nixpkgs-manual
  • postlight-parser
  • yarnInstallHook

@doronbehar
Copy link
Contributor

When attempt to use npmInstallHook on a yarn project using fetchYarnDeps, npm frequently tries to connect to the internet during the prune step.

Turns out replacing npm prune with yarn install --production=true ... resolves this issue.

Right now this is just a copy-paste of npmInstallHook, but I expect some other tweaks to be necessary, so I did not try to modify npmInstallHook to work with yarn.

Only now I noticed the code duplication (sorry for my lack of concentration). I wish we could avoid it, as I doubt we'd ever need to modify yarn packages' manual pages and binaries installation (based on jq parsing of package.json) differently from other npm packages.

Perhaps we can make this yarnInstallHook essentially run npmInstallHook with dontNpmPrune, and then run yarn install --production=true? Also, note how in principal you should mention (somehow) all variables inherited from npmInstallHook such as npmPackFlags, npmFlags which are also used by the new hook you add.

Another option would be to remove the npm prune command from npmInstallHook, and add an npmPruneHook. Then we should do something like this:

diff --git i/pkgs/build-support/node/build-npm-package/default.nix w/pkgs/build-support/node/build-npm-package/default.nix
index 4bcbf3f14f2f..ca5df6f0f372 100644
--- i/pkgs/build-support/node/build-npm-package/default.nix
+++ w/pkgs/build-support/node/build-npm-package/default.nix
@@ -55,6 +55,8 @@
 , npmBuildHook ? null
   # Custom npmInstallHook
 , npmInstallHook ? null
+  # Custom npmPruneHook
+, npmPruneHook ? null
 , ...
 } @ args:
 
@@ -74,6 +76,7 @@ stdenv.mkDerivation (args // {
       (if npmConfigHook != null then npmConfigHook else npmHooks.npmConfigHook)
       (if npmBuildHook != null then npmBuildHook else npmHooks.npmBuildHook)
       (if npmInstallHook != null then npmInstallHook else npmHooks.npmInstallHook)
+      (if npmPruneHook != null then npmPruneHook else npmHooks.npmPruneHook)
       nodejs.python
     ]
     ++ lib.optionals stdenv.isDarwin [ cctools ];

And add npmPruneHook manually to expressions that use npmInstallHook directly.

What do you think?

@lelgenio
Copy link
Contributor Author

Only now I noticed the code duplication (sorry for my lack of concentration). I wish we could avoid it, as I doubt we'd ever need to modify yarn packages' manual pages and binaries installation (based on jq parsing of package.json) differently from other npm packages.

Perhaps we can make this yarnInstallHook essentially run npmInstallHook with dontNpmPrune, and then run yarn install --production=true? Also, note how in principal you should mention (somehow) all variables inherited from npmInstallHook such as npmPackFlags, npmFlags which are also used by the new hook you add.

One thing I still need to figure out is if we maybe need to use yarn pack instead of npm pack (to be able to pass yarn flags reliably to all yarn* hooks), the installation of executables and manuals will probably stay the same.

If we are to share code between npmInstallHook and yarnInstallHook, we should probably not just inherit and instead be explicit in structuring the code in a way that screams "this code is used by more than just npmInstallHook".

Another option would be to remove the npm prune command from npmInstallHook, and add an npmPruneHook. Then we should do something like this:

diff --git i/pkgs/build-support/node/build-npm-package/default.nix w/pkgs/build-support/node/build-npm-package/default.nix
index 4bcbf3f14f2f..ca5df6f0f372 100644
--- i/pkgs/build-support/node/build-npm-package/default.nix
+++ w/pkgs/build-support/node/build-npm-package/default.nix
@@ -55,6 +55,8 @@
 , npmBuildHook ? null
   # Custom npmInstallHook
 , npmInstallHook ? null
+  # Custom npmPruneHook
+, npmPruneHook ? null
 , ...
 } @ args:
 
@@ -74,6 +76,7 @@ stdenv.mkDerivation (args // {
       (if npmConfigHook != null then npmConfigHook else npmHooks.npmConfigHook)
       (if npmBuildHook != null then npmBuildHook else npmHooks.npmBuildHook)
       (if npmInstallHook != null then npmInstallHook else npmHooks.npmInstallHook)
+      (if npmPruneHook != null then npmPruneHook else npmHooks.npmPruneHook)
       nodejs.python
     ]
     ++ lib.optionals stdenv.isDarwin [ cctools ];

And add npmPruneHook manually to expressions that use npmInstallHook directly.

What do you think?

You know how sometimes it feels more like you are discovering a solution rather than creating it?
Now that you mention it, it seems clear to me that there's a problem with npmInstallHook:
The behavior of pruning should be in a separate hook, probably called npmFixupHook, so it matches with a phase. "fixing" this is not vital to this PR though. (A draft: #335504)

Maybe we can create a "shared" set of these functions (installNodeMan, wrapNodePrograms or something to that effect?), use them in yarnInstallHook, create a yarnFixupHook too.
At first I was skeptical of making changes to npmInstallHook in this PR, but if I make it easily revertible (separate commit), it should be fine.

By now each hook would be tiny, and very specific, which is good I think. But there will be too many hooks for it to be reasonable for normal usage, I see a buildYarnPackage in the future..

@doronbehar
Copy link
Contributor

One thing I still need to figure out is if we maybe need to use yarn pack instead of npm pack (to be able to pass yarn flags reliably to all yarn* hooks), the installation of executables and manuals will probably stay the same.

That sounds more correct, and it will also justify separating the hooks.

Maybe we can create a "shared" set of these functions (installNodeMan, wrapNodePrograms or something to that effect?), use them in yarnInstallHook, create a yarnFixupHook too.

After speculating the idea suggested by me and implemented in #335504 , I was thinking of another option: Using Nix to share the code! Here's a draft:

@doronbehar
Copy link
Contributor

BTW Another option is to use the plural postInstallHooks, as done in the yarnConfigHook commit at #335751 . See 28e5a20 ( Hopefully this link won't die at some point).

@lelgenio lelgenio force-pushed the yarn-install-hook branch 2 times, most recently from e04554f to b7e195a Compare September 1, 2024 00:54
@lelgenio
Copy link
Contributor Author

lelgenio commented Sep 1, 2024

Fixed two things:

  1. name from package.json can contain special characters like /, so the output of yarn pack now goes to a fixed path in $TMPDIR.
  2. yarn pack does not work at all with bundleDependencies, from what I gather this feature was useful before lockfiles where a thing, we are providing all dependencies according to yarn.lock, so we can just ignore it by temporarily removing them from package.json

@lelgenio
Copy link
Contributor Author

lelgenio commented Sep 1, 2024

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

5 packages built:
  • blade-formatter
  • codefresh
  • nixpkgs-manual
  • postlight-parser
  • yarnInstallHook

@lelgenio
Copy link
Contributor Author

lelgenio commented Sep 1, 2024

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

5 packages built:
  • blade-formatter
  • codefresh
  • nixpkgs-manual
  • postlight-parser
  • yarnInstallHook

@doronbehar
Copy link
Contributor

There's a small merge conflict now BTW.

@doronbehar
Copy link
Contributor

I asked for help on Discourse.

@lelgenio lelgenio force-pushed the yarn-install-hook branch 2 times, most recently from 1dd7836 to c75d7cd Compare September 4, 2024 11:24
@lelgenio
Copy link
Contributor Author

lelgenio commented Sep 4, 2024

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

6 packages built:
  • blade-formatter
  • codefresh
  • element-call
  • nixpkgs-manual
  • postlight-parser
  • yarnInstallHook

@Aleksanaa Aleksanaa changed the title yarnInstallHook: init yarnInstallHook: init Sep 4, 2024
@doronbehar
Copy link
Contributor

@ofborg eval

@doronbehar
Copy link
Contributor

@NixOS/nixpkgs-vet something is wrong here I think.

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.

Would like to merge when CI is green - waiting for @NixOS/nixpkgs-vet 's response regarding the failure.

@philiptaron
Copy link
Contributor

Could you try rebasing on master right now and seeing if the error persists? The tree had some evaluation failures checked in earlier today.

@doronbehar
Copy link
Contributor

So I understand there is no equivalent to @ofborg eval for the nixpkgs-vet check?

@philiptaron
Copy link
Contributor

So I understand there is no equivalent to @ofborg eval for the nixpkgs-vet check?

Since it's "inside" GitHub actions instead of "outside" GitHub Actions, no, it gets fed a revision and it tests that revision only. It's not an active service or a bot in the same way that @ofborg is. It's just a tool that runs and reports.

@philiptaron philiptaron merged commit afb866e into NixOS:master Sep 4, 2024
26 of 30 checks passed
@philiptaron
Copy link
Contributor

I validated that this PR, when rebased on master, passes nixpkgs-vet.

@infinisil
Copy link
Member

It might be possible by clicking the "Re-run jobs" button in the workflow page. Can only be done by committers, and I'm only 90% convinced that it actually picks up fixed base branches. Please give it a try when you see it next time @philiptaron (too late for this PR though because the merge branch is already gone) :)

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.

7 participants