Here's the performance comparison between the old approach and the new one:

(only x86_64-linux, I've removed lines without diff and lines about time)

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"?

Any blockers preventing merging this?

IMHO no

Backport failed for release-25.05, because it was unable to cherry-pick the commit(s).

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?

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.
@vcunat This change only adds a field to meta, it doesn't change anything else about how packages are being handled, so it is safe to backport it to 25.05. The idea is to start using it with current release and gather as much feedback as possible.

Isn't this too intrusive for backporting?

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:

• It has no internal usage & is pure overhead at the moment
• The design looks overly memory intensive (way too many attrsets all over the place)

I'm not opposed to the feature as such, but it needs to go back to the drawing board.
I want to revert this before it goes into 25.11.

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 no internal usage & is pure overhead at the moment

It has external uses and was really expected even in 25.05. See #354012 for more context (also cc @nikstur here).

The design looks overly memory intensive (way too many attrsets all over the place)

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.

given how the previous attempts have gone

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.

bash fails to eval its CPE. Fixed in #459616.

@RossComputerGuy it looks like #435033 by @infinisil broke Bash CPE eval, it was fine after this PR.
I don’t quite understand why CI didn’t catch that before merge.

That is odd…

It’s off by default:

but it looks like it is enabled for the CI eval:

@adisbladis Before reverting what was brewing and collecting feedback for a long time, could you please provide more specific suggestions here?

It has no internal usage & is pure overhead at the moment

It has external uses and was really expected even in 25.05. See #354012 for more context (also cc @nikstur here).

The design looks overly memory intensive (way too many attrsets all over the place)

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.

You can't just say that something unrelated makes up for adding evaluation overhead. That's not how this works at all.
Using that logic I could go and introduce all sorts of performance regressions all over the place. Sounds great, eh?

@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 find your sense of urgency disturbing.. This is at the very core of nixpkgs and will seep in into the evaluation of every single derivation created.
Additionally this touches on API stability: The confidence of meta fields to be correct needs to be very high because we can't just go and break the API after the fact.

I think the entire design looks problematic. But I'll mention a few specific instances of why this should be reverted.

*/
tryCPEPatchVersionInUpdateWithVendor =
  vendor: version:
  let
    regex = "([0-9]+\\.[0-9]+)\\.([0-9]+)";
    # we have to call toString here in case version is an attrset with __toString attribute
    versionMatch = builtins.match regex (toString version);
  in
  if versionMatch == null then
    {
      success = false;
      error = "version ${version} doesn't match regex `${regex}`";
    }
  else
    {
      success = true;
      value = {
        inherit vendor;
        version = elemAt versionMatch 0;
        update = elemAt versionMatch 1;
      };
    };

This is going to cause a lot of intermediate attribute sets to be allocated and is going to be another instance of #269637.

defaultCPEParts = {
  part = "a";
  #vendor = null;
  ${if attrs ? pname then "product" else null} = attrs.pname;
  #version = null;
  #update = null;
  edition = "*";
  sw_edition = "*";
  target_sw = "*";
  target_hw = "*";
  language = "*";
  other = "*";
};

needs to be moved up in scope to not re-allocate for the same attrset on every invocation.

These design issues don't surface in benchmarks yet because this feature is barely used.

@adisbladis sorry for late reply

@adisbladis Before reverting what was brewing and collecting feedback for a long time, could you please provide more specific suggestions here?

It has no internal usage & is pure overhead at the moment

It has external uses and was really expected even in 25.05. See #354012 for more context (also cc @nikstur here).

The design looks overly memory intensive (way too many attrsets all over the place)

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.

You can't just say that something unrelated makes up for adding evaluation overhead. That's not how this works at all. Using that logic I could go and introduce all sorts of performance regressions all over the place. Sounds great, eh?

I'm sorry, I didn't convey the meaning correctly. We had that unoptimised code for quite some time, and nobody seemed to complain about it. Working on this PR, I found those optimisation opportunities and fixed them. I only landed them separately to have a clear picture of the performance impact of all the code related to CPEs. Overall work on CPEs made Nixpkgs evaluation faster. It doesn't mean that we can ignore potential issues in it, but it does mean that it is within the bounds of the performance that was, and still is tolerated.

In general, I find that trying to make things perfect before merging them can only lead to PRs being stuck in review purgatory, and features getting abandoned. I'm not saying that we should land completely unoptimised and hurtful code, especially in critical evaluation paths. I'm saying that we should land "good enough" code and optimise it while allowing users to start using new features, making Nixpkgs better for everybody.

@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 find your sense of urgency disturbing.. This is at the very core of nixpkgs and will seep in into the evaluation of every single derivation created. Additionally this touches on API stability: The confidence of meta fields to be correct needs to be very high because we can't just go and break the API after the fact.

The design of this feature includes versioning. This allows us to break API in the future in a safe manner, so if we find that the current API is too heavy, we can address that in the future.
As for urgency, this PR (well, the original one) was on review for about 3 months, following 6 months of discussions in the original issue. I don't believe this shows careless urgency, the feature is ready to be used, the design was mostly settled. I tried to implement it to the best of my ability, and several people reviewed it.
What makes you not so confident in this process?

I think the entire design looks problematic. But I'll mention a few specific instances of why this should be reverted.

*/
tryCPEPatchVersionInUpdateWithVendor =
  vendor: version:
  let
    regex = "([0-9]+\\.[0-9]+)\\.([0-9]+)";
    # we have to call toString here in case version is an attrset with __toString attribute
    versionMatch = builtins.match regex (toString version);
  in
  if versionMatch == null then
    {
      success = false;
      error = "version ${version} doesn't match regex `${regex}`";
    }
  else
    {
      success = true;
      value = {
        inherit vendor;
        version = elemAt versionMatch 0;
        update = elemAt versionMatch 1;
      };
    };

This is going to cause a lot of intermediate attribute sets to be allocated and is going to be another instance of #269637.

This code is only used when product field is set, but others aren't. I expect this to be a rare occurrence. It also doesn't affect meta API, so we can change it to a less abstract version later if needed.

defaultCPEParts = {
  part = "a";
  #vendor = null;
  ${if attrs ? pname then "product" else null} = attrs.pname;
  #version = null;
  #update = null;
  edition = "*";
  sw_edition = "*";
  target_sw = "*";
  target_hw = "*";
  language = "*";
  other = "*";
};

needs to be moved up in scope to not re-allocate for the same attrset on every invocation.

Note that this attrset depends on attrs, so it cannot be just moved up. We could move up the part that doesn't depend on attrs and only create a new attrset if pname is set (we'll have to ultimately create 2 attrsets to do so). I think this would produce more attrsets, because pname is set in majority of cases.

These design issues don't surface in benchmarks yet because this feature is barely used.

Please note that these are not design issues, but rather potential performance pitfalls. We can address these if they actually become problematic, after we can measure the actual usage and actual impact. Otherwise any of these optimisations can turn out bad for the common usecase in the long run. Remember that premature unmeasured optimisation can always become its own enemy.

I'd like to reiterate that when performance of some piece of code is bad, I find it better to fix performance issues rather than reverting features. Please let me know if you find any other issues with this code. If there is a way to optimise them, we can discuss it and implement it.