-
-
Notifications
You must be signed in to change notification settings - Fork 17.2k
Reapply "stdenv: Add CPE fields to meta" #439074
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
Conversation
|
Here's the performance comparison between the old approach and the new one:
(only |
|
A random quick thought: I suppose you did consider using e.g. the empty string instead of null, if placeholders are somehow preferable? |
|
@vcunat these parts can be empty, so empty string would not work as a placeholder. Anything other than null would be either much slower or less intuitive than just omitting them, I think. |
|
Ah, I didn't know they can be empty. Then it's clear, I guess. |
|
@YorikSar do you want that @infinisil approves this PR or what would be the next "step"? |
ConnorBaker
left a comment
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.
A few comments; otherwise looks good!
pkgs/stdenv/generic/check-meta.nix
Outdated
| makeCPE = | ||
| cpeParts: | ||
| "cpe:2.3:${cpeParts.part}:${cpeParts.vendor}:${cpeParts.product}:${cpeParts.version}:${cpeParts.update}:${cpeParts.edition}:${cpeParts.sw_edition}:${cpeParts.target_sw}:${cpeParts.target_hw}:${cpeParts.language}:${cpeParts.other}"; |
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.
| makeCPE = | |
| cpeParts: | |
| "cpe:2.3:${cpeParts.part}:${cpeParts.vendor}:${cpeParts.product}:${cpeParts.version}:${cpeParts.update}:${cpeParts.edition}:${cpeParts.sw_edition}:${cpeParts.target_sw}:${cpeParts.target_hw}:${cpeParts.language}:${cpeParts.other}"; | |
| makeCPE = flip pipe [ | |
| (attrVals [ | |
| "part" | |
| "vendor" | |
| "product" | |
| "version" | |
| "update" | |
| "edition" | |
| "sw_edition" | |
| "target_sw" | |
| "target_hw" | |
| "language" | |
| "other" | |
| ]) | |
| (concatStringsSep ":") | |
| ]; |
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.
Instead of doing the length check above, have you considered having makeCPE unpack the function arguments? That would ensure that both the length is correct and that the names are correct.
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.
Suggested change
That's just too many allocations and operations to add to each package evaluation.
having makeCPE unpack the function arguments
That's a nice idea. Will do. Although I would still check the length to avoid using exceptions as flow control.
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.
As far as I can tell the original issue that led to the revert has been fixed. Good to go from my end.
Thank you for the work @YorikSar! I think we will have to build some more experience to get the versions right, but this can only be done iteratively.
|
Any blockers preventing merging this? |
|
IMHO no |
This reverts commit de74f9c.
nix-env writes a warning for each derivation that has null in its meta values, so fields without known values are removed from the result. Fixes issue raised by @K900 in NixOS#409797 (comment)
|
@ConnorBaker Looks like you've caught this merge conflict as it was landing in master or smth. Rebased. |
|
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-439074-to-release-25.05 origin/release-25.05
cd .worktree/backport-439074-to-release-25.05
git switch --create backport-439074-to-release-25.05
git cherry-pick -x dd12290517ff6e57ab2ca384b101e0cdc617570c a178fd8c436915d233a1de37a2b211489aad5340 5e1eee582748866a51ca8f780829c3b2cf523fc3 |
|
Any chance you’d be able to do the backport? If not I’ll try to get around to it this weekend. |
|
Isn't this too intrusive for backporting? |
the backport can be set on hold, if you want to gather experience? The benefit of CPE's and pURL outweights the low-medium risk. 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 - which would have been unpatched for some time. In addition CVE like this #411881 were not detected properly, therefore an additional reason for backporting / benefiting today |
|
I've updated the previous backport in #438385 with new commits. Please take a look. |
I agree that we should not backport this. Since the 25.11 release cycle is starting, let's get the experience with this over the few months until that happens, and have 25.11 be the release where this becomes a Nixpkgs staple. |
|
This feature seems very half baked:
I'm not opposed to the feature as such, but it needs to go back to the drawing board. |
|
I would also very much prefer to see this land after 25.11, given how the previous attempts have gone. |
|
@adisbladis Before reverting what was brewing and collecting feedback for a long time, could you please provide more specific suggestions here?
It has external uses and was really expected even in 25.05. See #354012 for more context (also cc @nikstur here).
Could you please be more specific here? While the final implementation of this PR increases the total amount of attrsets needed for evaluation, several spinoff PRs reduced all these numbers significantly, which more than pays for this. Of course, it would be nice to keep optimising our eval times, but I think we should do this gradually, without reverting any new feature that needs more resources. You can't expect every PR to be perfectly optimised and not require any additional resources before it lands. This is just not how the world works. @K900 I'm not sure I follow your reasoning either.
Do you mean that because 1 (only one) previous attempt at landing this change broke evaluation, which was then fixed in this PR, this feature is doomed to be in revert-reapply purgatory? @adisbladis @K900 We need this feature to land in the release to start collecting data for vulnerability tracking initiatives. I find it very frustrating that you've decided that this needs to be reverted 2 months after it was merged with no proper review (apart from how "it looks") and with no time to address any feedback you might provide before the release. |
|
I think it is very important to land those extra meta information to make the security process easier and more automated which hopefully reduces churn on the security team and makes the OS more secure for everyone. |
This reverts commit de74f9c from #438527 that reverted #409797.
Added a change that removes all
nullvalues frommeta.identifiersto satisfynix-env's XML and JSON outputs.Unfortunately, this means that our API will have "holes" for fields with unknown values.
Add a 👍 reaction to pull requests you find important.