Why do we need two implementations of this at the same time?

I believe it's likely this could cause a world rebuild since I did remove a bunch of extraBuildCommands stuff to make it easier to maintain.

Then let's have a world rebuild?

Then let's have a world rebuild?

Done

Ok, I've done some cleaning up on the ordering and it should be closer to a readable diff. It falls apart when it gets to where libraries was removed but not much I can do there.

I would really like to see this down to 0 rebuilds. For a PR like this, every rebuild is a regression that we need to figure out. Even if the refactor uncovers previously‐incorrect behaviour or things that can be improved, it’s better to do those separately so that we can demonstrate that we can achieve the same behaviour through nicer mechanism without affecting the jobset (and avoid expensive rebuilds, in the case of ROCm). Can it not be minimized just by adjusting the overrides in the ROCm tree?

Definite +1 for using a proper scope here in general though.

Can it not be minimized just by adjusting the overrides in the ROCm tree?

It cannot as there's something with splicing going on because of how overrides change. The version attribute was different between the host or target, not entirely sure but that's what nix-diff was hinting at. I tried overriding it but ended up getting it to work in 1 way or another but not both to prevent rebuilds.

Is splicing involved here? My understanding was that ROCm doesn’t use the Nixpkgs cross functionality at all, so splicing should have no effect.

Is splicing involved here?

I think it's something that makeeScopeWithSplicing' changes when you override something which ROCm uses. But I'm not sure, it doesn't explain why before some derivations had the ROCm LLVM version and some had the standard LLVM 18 version. Now, all derivations have the ROCm LLVM version.

#440273 will solve the LLVM 12 and 13 rebuilds, at least. The eval comparison currently shows this PR as removing llvmPackages_19, which seems… weird?

As far as ROCm goes, it seems like the two options would be to change its overrides in this PR to produce the same result (e.g. maybe overriding buildPackages/targetPackages with the vanilla package set would do it?), or to change it before this PR to do the override in a way that produces the result it gets here. Maybe @LunNova has opinions here on what the better option would be. We can of course follow up by adjusting ROCm to do whatever makes the most sense afterwards, but it shouldn’t be too hard to get the rebuilds down to 0 here, and it’s important for validating that changes like this aren’t causing regressions.

Applying the extra commit in #440697 gets rid of the rocmPackages rebuild. If that seems like an acceptable way to unblock this it works for me. I can follow up with a rocmPackages rebuilding PR that drops the bug-for-bug rebuild avoidance.

The big whitespace diff makes it hard to tell; is it just overriding the inputs so that the overrides only happen at the top level? If so it seems reasonable to me.

The big whitespace diff makes it hard to tell; is it just overriding the inputs so that the overrides only happen at the top level? If so it seems reasonable to me.

I've split it into two parts to make it easier to view and attempted to minimize the number of touched lines.

• use overrideScope 5f06b99683a29192d0213b051b4181be443bb5b1
  • clang-unwrapped: continue to filter out and reapply patch instead of dropping now that the scope issues are fixed, note this should be removed shortly
  • compiler-rt-libc: pass incorrect libllvm to compiler-rt-libc to avoid rebuilding
• nix fmt 1071572b1387d175753d516d1f3bbf2945ad3838

If it's possible to structure this so nixfmt doesn't want to indent everything further and avoid the reformat commit that would be nice, I couldn't figure it out.

Yeah, that looks good to me, and adding it to this PR seems like a good fix.

I’m curious why you need the builtins.toString – does this somehow make postPatch become null sometimes or something?

I have no idea why nixfmt wants to split the function on another line. It seems to only happen because there’s two parameters? A very silly thing you could do would be overrideLlvmPackagesRocm = f: llvmPackagesRocm.overrideScope (final: prev: f { inherit final prev; }); and then overrideLlvmPackagesRocm (scope: … scope.final … scope.prev …). But actually, I think squashing these two commits shouldn’t be too bad, because you can choose “ignore whitespace” and it should give more or less the same result. Still, it’s unfortunate for git blame and so on. cc @NixOS/nix-formatting

I’m curious why you need the builtins.toString – does this somehow make postPatch become null sometimes or something?

On further investigation I shouldn't need to change that. I accidentally exposed new attributes from the original scope such as llvm-manpages, causing a failure to eval because of how those set postPatch to null. If I don't expose those attributes then there's no need to change this yet.

A very silly thing you could do would be overrideLlvmPackagesRocm = f: llvmPackagesRocm.overrideScope (final: prev: f { inherit final prev; });

That will make the ROCm 6.4.x bump PR less annoying for future me to rebase so I kinda prefer going with that.

Third attempt: 76144e608f19b1a83af3b2b531c418c486a4b2ad

Don't do that inherit. It does not share the override call and will macro-expsnd to doing it for every single attribute. You need a let. Yes this is horrible.

Don't do that inherit. It does not share the override call and will macro-expsnd to doing it for every single attribute. You need a let. Yes this is horrible.

augh. Can I do something like below or do I need a large pile of a = overridenScope.a; b = ...?

let overridenScope = (llvmPackagesRocm.overrideScope (final: prev: f { inherit final prev; })); in {
  inherit (overridenScope)
    a
    b
    ...
    ;
}

Yeah that's fine, since it just desugars to the same pile.

Fixed in 47393c7

Is it a problem that there are a lot of inherit (callPackage… in all-packages.nix? 😨

Yes, I think so :( But really it’s just a Nix implementation problem and should ideally be fixed there.

Don't do that inherit. It does not share the override call and will macro-expsnd to doing it for every single attribute. You need a let. Yes this is horrible.

@emilazy I thought this had been fixed since Lix 2.90 and CppNix 2.21. Is this concern just for those still running CppNix <2.21?

Hmm, I was even just glancing at that code a few days ago but didn’t realize it seems to do the right thing now. That’s nice. Maybe it’s fine, then.

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

https://discourse.nixos.org/t/how-to-modify-cmakeflags-in-a-library-in-llvmpackages-in-a-cross-compile-flake/69644/2

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

https://discourse.nixos.org/t/overriding-llvmpackage-source-with-local-checkout-in-flake-for-clang-tools-package/67502/17

This was included in #445668 which was merged.