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

prefetch-yarn-deps: Fix handling of scoped packages #257337

Merged
merged 2 commits into from
Nov 26, 2023

Conversation

lorenzleutgeb
Copy link
Member

Description of changes

I ran into an issue with a lockfile entry of the form "@org/somepack@file:vendor/orgpacks/somepack/assets". fetch-yarn-deps would crash, because it would not detect @file:... and just blindly access pkg.resolved, which was not present.

Things done

Diagnosed and fixed the bug, added a test to avoid regressions in the future.

  • 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.11 Release Notes (or backporting 23.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.

@lorenzleutgeb
Copy link
Member Author

@ofborg build gitmoji-cli marp

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2716

@lorenzleutgeb
Copy link
Member Author

lorenzleutgeb commented Oct 16, 2023

@lilyinstarlight maybe this is worth your attention before 23.11 branch-off.

@lilyinstarlight lilyinstarlight added the backport release-23.11 Backport PR automatically label Nov 21, 2023
@lilyinstarlight
Copy link
Member

lilyinstarlight commented Nov 21, 2023

@ofborg eval
@ofborg build prefetch-yarn-deps prefetch-yarn-deps.tests

Requesting a fresh eval and also fetcher test build, since this eval is a bit stale and because the commit message uses fetch-yarn-deps instead of prefetch-yarn-deps, ofborg doesn't build the tests

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.

The diff itself looks fine to me and does not appear to be a breaking change, so if ofborg builds pass and @yu-re-ka has no objections, I think this looks fine to merge

Thank you!

@lilyinstarlight
Copy link
Member

Ofborg failed because fetcher should probably set SSL_CERT_FILE like the others, now that #257513 was merged and Node.js no longer uses the built-in ones... (and this probably means all yarn fetchers are failing/unreproducible FODs right now...)

I'll open a separate PR to fix that since it is orthogonal to this PR

@lilyinstarlight
Copy link
Member

I've opened #269061 to fix the fetcher failures

@lorenzleutgeb
Copy link
Member Author

@ofborg eval
@ofborg build prefetch-yarn-deps prefetch-yarn-deps.tests

[...] because the commit message uses fetch-yarn-deps instead of prefetch-yarn-deps, ofborg doesn't build the tests

Fixed.

@lorenzleutgeb lorenzleutgeb changed the title fetch-yarn-deps: Fix handling of scoped packages prefetch-yarn-deps: Fix handling of scoped packages Nov 22, 2023
@lilyinstarlight lilyinstarlight merged commit 25596bd into NixOS:master Nov 26, 2023
23 checks passed
Copy link
Contributor

Successfully created backport PR for release-23.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants