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

Switch from mkYarnPackage to yarn{Config,Build}Hook. #324246

Open
19 of 46 tasks
doronbehar opened this issue Jul 3, 2024 · 8 comments
Open
19 of 46 tasks

Switch from mkYarnPackage to yarn{Config,Build}Hook. #324246

doronbehar opened this issue Jul 3, 2024 · 8 comments
Labels
3.skill: sprintable A larger issue which is split into distinct actionable tasks 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: nodejs

Comments

@doronbehar
Copy link
Contributor

doronbehar commented Jul 3, 2024

After #318015 will be Now that #318015 is merged, we should start deprecate the usage of mkYarnPackage, because it is too complex, and hard to maintain. It also requires to vendor into Nixpkgs a package.json file for each such package (to avoid IFD, see #296856 ).

I'm opening this issue before #318015 is ready / merged to refer to this issue in a small documentation I will perform there.

The following list was generated with:

env NIXPKGS_ALLOW_INSECURE=1 nix eval --json -L --impure --expr '
  let
    pkgs = (builtins.getFlake "github:nixos/nixpkgs").legacyPackages.${builtins.currentSystem}; 
  in builtins.map (pPath: let
      p = builtins.baseNameOf (builtins.dirOf pPath);
    in if builtins.hasAttr p pkgs then
      if builtins.hasAttr "maintainers" pkgs.${p}.meta then 
        "- [ ] `${p}`: ${builtins.concatStringsSep " " (builtins.map (m: "@${m.github}") pkgs.${p}.meta.maintainers)}"
      else 
        "- [ ] `${p}`: no maintainers found"
    else
      "- [ ] `${p}`: did not found attribute automatically, file is `${pPath}`"
  ) [
    '"$(git grep -l mkYarnPackage pkgs | while read p; do echo \"$p\"; done)"'
  ]' | jq --raw-output '.[]'
@gador
Copy link
Member

gador commented Jul 3, 2024

Is there a form of migration guide to use the new hooks?

I am using some scripted hacks to get the upstream yarn file, which also needs to be converted to the older v1 file to be used in nixpkgs. There are some dirty hacks involved, specific to pgadmin4, which would have to be used in the main derivation and not in the seperate update.sh script which right now will commit the fixed and ready to use yarn.lock file.
(https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/pgadmin/update.sh#L8-L16)

Also mkYarnModules did not work for pgadmin4 which needed special treatment to build the frontend and also uses the yarn webpacker. (https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/pgadmin/default.nix#L110-L122)

Is there a way to not use the newly introduced hooks? Is this even desired? Or will I have to find some way to run my specially crafted update.sh and preBuild yarn magic with the hooks?

@doronbehar
Copy link
Contributor Author

doronbehar commented Jul 3, 2024

Thanks for willing to cooperate @gador . Indeed this can become complex... The new hooks I added indeed usually makes things a bit simpler, but in such a complex case as you describe, you will have to play with it yourself, and there is no migration guide. However, there are many migration examples in #318015 .

The non-trivial update scripts were discussed a bit in #318015 . I believe that with the new hooks you should be able to reach a point where you don't vendor any file (neither yarn.lock or package.json), and you should also be able to update your package with nix-update which is capable of updating the hash of the offlineCache by itself.

@doronbehar doronbehar added the 3.skill: sprintable A larger issue which is split into distinct actionable tasks label Jul 3, 2024
@gador
Copy link
Member

gador commented Jul 3, 2024

Thanks for all your work @doronbehar !
I looked through your examples. AFAI can see I could rely on my update script and build script as it is now and not use the newly introduced hooks and everything should continue to work fine. I don't use mkYarnPackage anyway. However I will look into it if there is a way to use the hooks and maybe get rid of the packaged yarn.lock file in the future!

@pyrox0 pyrox0 added the 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems label Jul 5, 2024
@lelgenio
Copy link
Contributor

I think we'll also need a yarnInstallHook: #328544

@pyrox0
Copy link
Member

pyrox0 commented Aug 14, 2024

koodoo-reader, listmonk, spectral-language-server, and treedome were all migrated in #318015

Further, typst-preview can be removed from the list, as it was removed in #325275.

@tengkuizdihar
Copy link
Contributor

tengkuizdihar commented Aug 14, 2024

thank you for migrating my package, sorry I didn't help in the migration

@gcoremans
Copy link

Do yarnConfigHook and yarnBuildHook support yarn v2 and up lockfiles?

@lelgenio
Copy link
Contributor

Do yarnConfigHook and yarnBuildHook support yarn v2 and up lockfiles?

No, they rely on the same fetchYarnDeps that was used by mkYarnPackage.

Here's an issue about that: #254369

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 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: nodejs
Projects
None yet
Development

No branches or pull requests

7 participants