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.

Sign up for GitHub

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

Jump to bottom

prefetch-npm-deps: download dev deps for git deps with install scripts #206476

Merged
merged 5 commits into from
Apr 25, 2023
Merged

prefetch-npm-deps: download dev deps for git deps with install scripts #206476

merged 5 commits into from
Apr 25, 2023

Conversation

winterqt
Copy link
Member

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@winterqt winterqt mentioned this pull request Dec 17, 2022
Closed
13 tasks
@winterqt
Copy link
Member Author

I wonder if I should combine this and #206477... hm.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Dec 17, 2022
lilyinstarlight
lilyinstarlight reviewed Dec 29, 2022
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I made a couple of nit comments, but overall the changes look good and function correctly in my testing

Edit: Also, the commit has installl with 3 l's when it should just be install

pkgs/build-support/node/fetch-npm-deps/src/parse/mod.rs Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/parse/mod.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/default.nix Outdated Show resolved Hide resolved
@winterqt winterqt force-pushed the prefetch-npm-deps-git-dev-deps branch from b25e2e6 to f6000ab Compare December 29, 2022 05:25
@winterqt winterqt marked this pull request as draft December 29, 2022 05:26
@winterqt
Copy link
Member Author

Pushed the fixes as separate commits so you can more easily review them.

@winterqt winterqt force-pushed the prefetch-npm-deps-git-dev-deps branch from 2678aa9 to 99efa90 Compare January 31, 2023 03:09
@winterqt winterqt marked this pull request as ready for review January 31, 2023 03:10
@winterqt
Copy link
Member Author

Okay, we should be good to go with this set of changes. Apologies for the delay as always.

@lilyinstarlight Please take a look, especially at the commit messages. Thanks.

winterqt added a commit to winterqt/nixpkgs that referenced this pull request Feb 1, 2023
@winterqt
prefetch-npm-deps: throw better error when unsupported git service is…
208a4d0 
… used

See NixOS#206476 (comment).
winterqt added a commit to winterqt/nixpkgs that referenced this pull request Feb 4, 2023
@winterqt
prefetch-npm-deps: throw better error when unsupported git service is…
66c074f 
… used

See NixOS#206476 (comment).
lilyinstarlight
lilyinstarlight reviewed Apr 5, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Okay so this looks good to me, but it seems to have broken the mongosh FOD

I haven't dived into why yet but it may just be that it needs to be updated because it wasn't generating right to begin with (and I guess it maybe worked by chance since it's built from a workspaced repo and doesn't need all locked deps idk). Regardless I'll investigate hopefully tomorrow so that this can get merged

I manually checked all other FODs and nixpkgs-review came in with no new regressions over master

Edit: The mongosh FOD hash changed becuase it wasn't downloading dev dependencies from https://github.com/mongodb-js/saslprep/tree/v1.0.4 (a transitive dependency) and so since including git dep lockfiles is the purpose of this PR, I will say that is Working As Intended and update the hash

@winterqt @lilyinstarlight
prefetch-npm-deps: refactor
3e68a27 
This splits prefetch-npm-deps into multiple files, as well as making a
few small changes along the way, such as going from a `HashMap` to a `Vec`
as the container for packages, to deduplicate them more efficently.
@winterqt @lilyinstarlight
prefetch-npm-deps: download dev deps for git deps with install scripts
7e247e6 
Git dependencies with install scripts are built isolated from the main
package, so their development dependencies are required.

To take advantage of this, NixOS#206477 is needed.
@winterqt @lilyinstarlight
prefetch-npm-deps: throw better error when unsupported git service is…
e1d64c1 
… used

See NixOS#206476 (comment).
@winterqt @lilyinstarlight
prefetch-npm-deps: fix clippy lint
faa3de1
@lilyinstarlight lilyinstarlight changed the title prefetch-npm-deps: download dev deps for git deps with installl scripts prefetch-npm-deps: download dev deps for git deps with install scripts Apr 6, 2023
@lilyinstarlight
mongosh: fix hash now that all git deps are fetched
a7391dd 
This FOD hash changed becuase it the fetcher was not downloading dev
dependencies from https://github.com/mongodb-js/saslprep/tree/v1.0.4 (a
transitive dependency) and so since including git dep lockfiles is the
purpose of these changes this change is intended and the hash should be
updated.

Generally a package was unable to build without these so I did not
expect this to be a breaking change (since there would be no existing
FODs), but mongosh seems to have been an exception. I think just
changing the hash here is more appropriate for this exception rather
than working in machinery for supporting old and new FODs (since it's
rather unlikely these prefetch-npm-deps changes will break downstream
code as it should not have worked in the first place if it needed this).
@lilyinstarlight
Copy link
Member

I've done one push to rebase and another push to update the mongosh FOD hash (see commit message for why)

I think this should be good now, but I'll do more tests and solicit another review so I'm not self-reviewing

@winterqt
Copy link
Member Author

@ofborg eval

RaitoBezarius
RaitoBezarius approved these changes Apr 23, 2023
View reviewed changes
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

This is awesome. :)

lilyinstarlight
lilyinstarlight approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

I did one final pass and everything looks good, with no regressions against master (4 failing builds are also all failing in Hydra)

Merging! 🎉 🚀

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

4 packages failed to build:
69 packages built:
  • ansible-language-server
  • authelia
  • bitwarden
  • cdxgen
  • element-desktop
  • element-desktop-wayland
  • emscripten
  • evcc
  • faust (faust2)
  • faust2alqt
  • faust2alsa
  • faust2csound
  • faust2firefox
  • faust2jack
  • faust2jackrust
  • faust2jaqt
  • faust2ladspa
  • faust2lv2
  • faustPhysicalModeling
  • faustlive
  • guitarix
  • hred
  • iosevka
  • iosevka-comfy.comfy
  • iosevka-comfy.comfy-duo
  • iosevka-comfy.comfy-fixed
  • iosevka-comfy.comfy-motion
  • iosevka-comfy.comfy-motion-duo
  • iosevka-comfy.comfy-motion-fixed
  • iosevka-comfy.comfy-wide
  • iosevka-comfy.comfy-wide-duo
  • iosevka-comfy.comfy-wide-fixed
  • kapitonov-plugins-pack
  • kaufkauflist
  • lv_img_conv
  • magnetophonDSP.CharacterCompressor
  • magnetophonDSP.CompBus
  • magnetophonDSP.LazyLimiter
  • magnetophonDSP.MBdistortion
  • magnetophonDSP.RhythmDelay
  • magnetophonDSP.VoiceOfFaust
  • magnetophonDSP.faustCompressors
  • magnetophonDSP.pluginUtils
  • magnetophonDSP.shelfMultiBand
  • matrix-appservice-irc
  • mongosh
  • mooSpace
  • mpvScripts.webtorrent-mpv-hook
  • open-music-kontrollers.mephisto
  • open-stage-control
  • paperless-ngx
  • photoprism
  • pnpm-lock-export
  • prefetch-npm-deps
  • schildichat-desktop
  • schildichat-desktop-wayland
  • sharing
  • sunshine
  • tambura
  • terminal-stocks
  • torq
  • twspace-crawler
  • uptime-kuma
  • vieb
  • vsce
  • webcord
  • zigbee2mqtt
  • zinc
  • zrythm

@lilyinstarlight lilyinstarlight merged commit 6fd6ce1 into NixOS:master Apr 25, 2023
@winterqt winterqt mentioned this pull request Nov 22, 2024
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

@lilyinstarlight lilyinstarlight lilyinstarlight approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@winterqt @lilyinstarlight @RaitoBezarius