-
-
Notifications
You must be signed in to change notification settings - Fork 17.1k
stdenv: pURL implementation #421125
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
stdenv: pURL implementation #421125
Conversation
0148e30 to
01d9064
Compare
|
Ping @pombredanne ^^ |
588627d to
b460a14
Compare
8c86e6e to
c253ef7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go 🎉
Can we merge this or did I miss something in the long thread?
@SuperSandro2000 yes please go ahead and merge, @emilazy asked @YorikSar to take another look and he did that yesterday evening. thank you! |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-25.05
git worktree add -d .worktree/backport-421125-to-release-25.05 origin/release-25.05
cd .worktree/backport-421125-to-release-25.05
git switch --create backport-421125-to-release-25.05
git cherry-pick -x 4e2614fc0709ff77e40a8f39e2744239ee371826 0a69474ed34ef6a4e82804b4b2d844deb126a1ab 2e46d00d76d3c9690e9713a9c2686c328e3779da c78e6a235962eb272981ea6b16939034c0fde575 64a6ca1114355caca991817cba83c4beb18136e2 22dbee80107516b858abd3d7a45c149a316a78d8 1f173d017207dc039a1c2494fd88c20d757d864c cadcde9f7f04c239c0e187903d524ae57afce569 25f90d7d20c46acd8eca5a8bf1b7f558e0efda02 83b6d2e657e2bbc19d55c48b0a888988014ac805 87977474f1802bb0a5dbc1e5ad60ce7f04624cc7 81dc446ee36274f737a05755af92b74e70e0c07d 3ddee85a175472d063063a3423524f668ed31b86 f7cbf2374b500cc2b87dbba11baa9b4ea03d6086 bacccc39a9cfd80b62940002f0c656add2aa3619 028af7c17dacf56953cafd8a19aaecd12edf7921 0ef545933fb1a707b70cb94b475a07343aa9ae7e |
|
I might have merged this too eagerly. I think we should've probably squashed the commits into one commit prior to merging to ease backporting. I'm sorry 😔 |
|
Now that we’re already in 25.11 release cycle I don’t think backport makes much sense. |
|
@h0nIg Please squash commits that address review feedback in future for the sake of bisectability and clean history, per the commit conventions linked in the PR checklist. |
|
This broke nixpkgs-review on aarch64-linux with a thunderbird-bin error, because on aarch64-linux there's no thunderbird-bin, so thunderbird-bin.src errors. I think nixpkgs-review tries to grab the full meta which then errors because meta now depends on src. Yes this really is the only package that has this problem. To reproduce: How should we fix this? |
thunderbird-bin is not supported on aarch64-linux, is this really a problem? Will check anyhow
nixpkgs/pkgs/applications/networking/mailreaders/thunderbird-bin/default.nix Lines 40 to 46 in d81b27c
|
|
Imo, that should be throwing an unsupported platform error rather than a missing attribute error. |
|
(copied from Matrix): I'm skeptical that this only happens for thunderbird-bin. I think there are plenty of packages where a src is not available on every platform. I assume that one can't read meta for these packages on other platforms now anymore. That seems quite bad. (I did not really look into this PR, but reading meta must be possible, even on unsupported platforms!) |
you can read thunderbird-bin.meta, but you can not read thunderbird-bin.meta.identifiers |
Since |
|
In practice this really breaks nixpkgs-review, so something must be fixed |
|
So the actual thing this breaks is AFAICT the thunderbird-bin variants are the only packages affected, but note that that doesn't mean it's the right solution to fix thunderbird-bin. It might mean we shouldn't have made meta depend on src to begin with, or maybe we should have been checking this in CI, or... |
I would like to summarize the 2 cases we hit:
We should thank @wolfgangwalther taking care about CI, he outlined conditions how this change should get included for the brought audience next time: #453322 (comment) I would like to hear @YorikSar, which gave his feedback and which encouraged me to move the logic into mkderivation and which offers the feature for everyone / languages, which was a bit too ambitious. options which i see, to get this included into 25.11.: option 1: go back to previous implementation 8797747 -1 commit, where we explicitly configure languages which are known to play by the rules. We lose general support with this - but we prevent issues which caused the need of the revert everyone: WDYT? |
Even with the previous approach, we're introducing this dependency between I guess, we could start by just adding PURLs to |
This reverts commit e47455f.
|
lets try another round, this time I would like to prevent fallout through a feature flag and still enable people to gather experience & maintain their non default data (e.g. jq example, where fetchurl is used instead of fetchFromGithub) - as outlined by @YorikSar above regarding the fetchers aspect only |
purl implementation based on drv.src, which will get referenced by the main derivation (if not specified otherwise)
Backport reasons for 25.05
In my opinion, this change can get backported to 25.05 as well (without the release note). Otherwise we meet again in 5 + x months (guessing 5+6 = 11, 1 year ahead), after maintainers start adopting their derivations and later start thinking about further enhancements.
My company (using nixpkgs 25.05) is actively using pURL and can give the learnings back to the community already today (once merged to 25.05).
In addition CVE like this jq CVE were not detected properly, therefore an additional reason for backporting / benefiting today:
The security tracker https://discourse.nixos.org/t/nixpkgs-supply-chain-security-project/34345/30 can benefit as well, because once a regular bump of software on master is done (without keeping a CVE in mind and determining "stable needs a fix as well"), nixpkgs stable fix is not triggered automatically.
Example: #409300 libarchive was bumped without CVE reference (edit to the PR description was adjusted after additional issue was created ONLY), just by coincidence it was backported as well.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.