Skip to content

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Jun 7, 2024

Motivation for this change

Many packages that use fetchYarnDeps (104 according to git grep), need to run the commands introduced by these new hooks, or variants of them. Any project with a yarn.lock a contributor may wish to package, essentially will need to run some variant of these commands. It'd be better to share this functionality, document it, and hence expose it.

Further more, as discussed with @yu-re-ka at #296856 , mkYarnPackage has issues, and should be replaced by hooks.

Other people who may be interested in this:

@SuperSandro2000 @lilyinstarlight @Scrumplex ( ❤️ )

Things done / TODO
  • Fix TODOs in hooks.
  • Documentation.
  • Release notes.
    external source.
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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/)
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label Jun 7, 2024
@doronbehar doronbehar changed the title vim-language-server: use yarnBuildHook Create yarnBuildHook and yarnConfigHook Jun 7, 2024
@doronbehar doronbehar force-pushed the pkg/yarnConfigHook branch from 7f0f1ec to 02116e6 Compare June 7, 2024 13:53
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Jun 7, 2024
@SuperSandro2000
Copy link
Member

Just wanted to say that I am very thankful that you are working on this and trying to get this into shape. 🎉

--offline --frozen-lockfile \
--force --production=false \
--ignore-engines --ignore-scripts
patchShebangs node_modules
Copy link
Member

Choose a reason for hiding this comment

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

While we are reworking things. We cannot limit this to node_modules/.bin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we are reworking things. We cannot limit this to node_modules/.bin?

What would be the value in trying that out? Here's an example of a patchShebangs log:

node_modules/acorn/bin/acorn: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/cross-spawn/node_modules/which/bin/node-which: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/findup/bin/findup.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/browserslist/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/envinfo/dist/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/errno/build.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/errno/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/esprima/bin/esvalidate.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/esprima/bin/esparse.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/flat/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/import-local/fixtures/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/he/bin/he: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/js-yaml/bin/js-yaml.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/json5/lib/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/mkdirp/bin/cmd.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/mocha/bin/_mocha: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/mocha/bin/mocha: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/mocha/lib/cli/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/semver/bin/semver: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/terser/bin/terser: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/terser/bin/uglifyjs: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/ts-loader/node_modules/semver/bin/semver.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/tslint/bin/tslint: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/ts-node/dist/bin-script-deprecated.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/ts-node/dist/bin-script.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/ts-node/dist/bin-transpile.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/ts-node/dist/bin.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/typescript/bin/tsc: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/typescript/bin/tsserver: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/webpack/bin/webpack.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/webpack-cli/bin/cli.js: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"
node_modules/which/bin/which: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/6g9n96qf1yx139xklnmy3v4xhjvjgsji-nodejs-20.12.2/bin/node"

None of the paths above includes node_modules/.bin...

Copy link
Member

Choose a reason for hiding this comment

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

or a pattern like node_modules/*/bin. They seem to be all over the place...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or a pattern like node_modules/*/bin. They seem to be all over the place...

It's worth checking in anycase whether this patchShebangs is really needed. I'll run a nixpkgs-review on a strong machine with and without it when I finish iterating all packages.

Copy link
Member

Choose a reason for hiding this comment

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

Uh, that would be super awesome if we can avoid carrying this forward with us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly to my comment to @viraptor here, I think it should be out of scope for this PR to remove that patchShebangs command, since this is also the behavior of all of these packages before this PR.

This also helps me demonstrate in the little diff --recursive shell loop below how the packages barely change due to this PR. I still kept the TODO comment near there though - maybe someone will be bothered by this in the future and provide a clean solution.

@doronbehar doronbehar force-pushed the pkg/yarnConfigHook branch 2 times, most recently from a335fac to 9ef9a41 Compare June 10, 2024 06:54
@SuperSandro2000
Copy link
Member

Just to confirm: did you check for potential update scripts where we can drop dowloading the package.json file fron upstream?

@doronbehar doronbehar force-pushed the pkg/yarnConfigHook branch from 9ef9a41 to 80965c7 Compare June 10, 2024 11:33
@doronbehar
Copy link
Contributor Author

Just to confirm: did you check for potential update scripts where we can drop dowloading the package.json file fron upstream?

Good comment. The files I touched in the meantime don't have an updateScript, but those who do are:

pkgs/applications/office/micropad/update.sh
pkgs/servers/home-assistant/custom-lovelace-modules/zigbee2mqtt-networkmap/update.sh
pkgs/servers/jellyseerr/update.sh
pkgs/servers/matrix-appservice-discord/update.sh
pkgs/tools/admin/meshcentral/update.sh

And most of them do download the package.json file and sometimes even yarn.lock. I noted this list in the TODO of the first comment of the PR.

@doronbehar doronbehar added the 3.skill: sprintable A larger issue which is split into distinct actionable tasks label Jun 24, 2024
@doronbehar doronbehar force-pushed the pkg/yarnConfigHook branch from 80965c7 to 36e0652 Compare July 3, 2024 07:58
@doronbehar doronbehar force-pushed the pkg/yarnConfigHook branch from 1827318 to 238d267 Compare July 10, 2024 06:40
@doronbehar
Copy link
Contributor Author

Thanks @pinpox for running nixpkgs-review! Many of the new packages are built again because electron is changed in this PR due to the above commented change. Other then that, the packages that I directly changed to use the hooks (those that appear in the commit log) have been tested by myself.

That redisinsight seems suspicious, I'll test shortly if that is failed on master as well. The Hydra web interface (and hydra-check as well) don't show this job... ( https://hydra.nixos.org/eval/1807565?filter=redisinsight )

I also fixed the merge conflict.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 10, 2024
@doronbehar
Copy link
Contributor Author

redisinsight also fails to build on master...

@doronbehar
Copy link
Contributor Author

Now with the additional approval, I'll merge this in a few days if no one will comment on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.skill: sprintable A larger issue which is split into distinct actionable tasks 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 8.has: changelog This PR adds or changes release notes 8.has: clean-up This PR removes packages or removes other cruft 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants