replaceVars: work around content-addressed output path shenanigans

[timschumi]

The placeholder output paths in content-addressed derivations lack the derivation name (NixOS/nix#12361), which breaks the general assumption that we could filter them using said name (e.g. when adjusting patch sets).

This is not really the fault of replaceVar, but the combination of it together with a patch seems common enough that we can just work around this here without many unneeded ill effects.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[LunNova]

Is this a common issue? rocmPackages' llvm is only doing the path filtering as a workaround for #375431 - it removes and then reapplies the same patch to avoid a non-overridden LLVM sneaking in.

[timschumi]

I'm not sure if the issue would be considered "common", but it certainly is an issue. However, I'd expect that most cases of patches being filtered are hidden in out-of-tree in nixpkgs overlays.

That said, if there is another possible workaround to this that I missed that would be considered less intrusive then I'm all ears, because on my testing configuration this patch causes ~2700 derivations to be reevaluated.

[philiptaron]

Let's fix this in the rocmModules definition. Much of what I read in pkgs/development/rocm-modules/6/llvm/default.nix makes me believe that #421201 ought to be merged. Since this is all code within Nixpkgs, surely we can concoct a better interface between the LLVM derivations and the ROCm transformations on them than observing the store paths. Turning off content-addressed derivation support for replaceVars instead seems backwards to me.

[philiptaron]

This meta-issue doesn't actually explain the behavior here described. Is there a more specific issue?

[timschumi]

I haven't seen anything that would be more specific than "this output path has a weird format", and this seemed to have been the only issue relating to this in the nix namespace. But probably a moot discussion point anyway, since we ideally want to fix this without disabling the described behavior.

[wolfgangwalther]

Let's fix this in the rocmModules definition. [...] Turning off content-addressed derivation support for replaceVars instead seems backwards to me.

Very much agree.

[LunNova]

Sorry for that mess :c Fixing it there makes more sense to me too.

I think it'd only make sense to apply a workaround in replaceVars if running into this in out-of-tree overrides is common and preventing people testing enabling content addressing by default.

[philiptaron]

There's nothing about replaceVars that conflict with content addressed derivations. What conflicts is existing derivation code (in this case, the override named above) that makes assumptions about how store paths are named, and locks in the pre-CA naming algorithm. Hyrum's law applies. This build helper is an innocent bystander.

As I read it, anyway.

[timschumi]

Closing in favor of whatever other solution we can come up with.

[timschumi]

I think it'd only make sense to apply a workaround in replaceVars if running into this in out-of-tree overrides is common and preventing people testing enabling content addressing by default.

If ROCm is indeed the only group of packages that does this then I suppose that is preferable, I don't have the experience or the Nix grepping skills to confirm that, so I opted for putting up the all-encompassing solution for debate. For out-of-tree overrides we probably don't have very accurate metrics, right?

This build helper is an innocent bystander. As I read it, anyway.

Indeed it is, the original description also mentions that all of this really isn't replaceVars fault.

[LunNova]

#436350 changes the LLVM package set to be an overrideable scope which will allow overrides on top of it not to rely on filtering out and reapplying patches, probably resolving this issue. Does that sound good @timschumi?

[timschumi]

#436350 changes the LLVM package set to be an overrideable scope which will allow overrides on top of it not to rely on filtering out and reapplying patches, probably resolving this issue. Does that sound good @timschumi?

I'm not up-to-date enough on overridable scopes to judge that, from looking at the PR I can only tell that neither findClangNostdlibincPatch nor its usage is removed, so I assume that PR alone won't yet fix it by itself. If adding overridable scopes adds some magic behavior (that I am not aware of) that does not require writing out the full patch list verbatim in ROCm then I guess it will resolve the issue, yes.

[timschumi]

That said, I'll obviously be happy to retest as required, since I have the build output locally already.