cmake: remove fixCmakeFiles

[SuperSandro2000]

Rewriting /usr and /opt to /var/empty is no longer necessary since the sandbox was introduced. It also introduced unexpected side effects and changes paths like $out/etc/opt/ to $out/etc/var/empty/

Closes #24215

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[bjornfor]

Nice, this should fix #24215.

[vcunat]

I'm not a darwin person, but AFAIK sandbox is still not enabled in there by default and /usr and /opt paths seem in use on those systems (for non-nix stuff).

[vcunat]

On *-linux I wouldn't expect significant fallout from this, at least.

[vcunat]

Actually, on *-linux I see risk for use cases like nix-shell on non-NixOS.

[nixos-discourse]

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

https://discourse.nixos.org/t/making-a-development-flake-for-bespoke-synth/29988/19

[SuperSandro2000]

Actually, on *-linux I see risk for use cases like nix-shell on non-NixOS.

hmmm, not sure if we can just chance that and hope for the best?

I'm not a darwin person, but AFAIK sandbox is still not enabled in there by default and /usr and /opt paths seem in use on those systems (for non-nix stuff).

Can someone more firm in darwin comment on that?

[reckenrode]

MacPorts and Homebrew (aarch64-darwin) both put stuff in /opt. Homebrew (x86_64-darwin) puts stuff in /usr/local. The sandbox is not enabled by default on Darwin, so my concern would be that those things could get picked up accidentally without it.

[SuperSandro2000]

Actually, on *-linux I see risk for use cases like nix-shell on non-NixOS.

I think this is acceptable as this was creating lots of subtle bugs and sometimes evening installing files into void.

The sandbox is not enabled by default on Darwin, so my concern would be that those things could get picked up accidentally without it.

but that's also true for any other PATH and those lines of code caused probably more breakages than they fixed and didn't work like expected in the first place anyway.

Also aren't the builds run under a different user even outside the sandbox? If that can't write to there, we should be safe, otherwise this seems like a big glaring issue that affects darwin in general.

[K900]

I really just want to kill it. Can we please kill it.

[ElvishJerricco]

@K900 It is not acceptable to make things worse on Darwin just because you don't care.

[Samasaur1]

Given that the sandbox is off by default on Darwin (which seems to be because it doesn't work well enough), this PR would introduce a huge amount of impurities on Darwin. This would be the case not only for users who have Homebrew and/or MacPorts installed alongside Nix, but for any Darwin user, since the system puts files in /usr as well.

To me, this seems like a change that would be a huge detriment to users of Nix on Darwin while providing comparatively little benefit to other Nix users

[K900]

I think I know why the hook is there now. These are the results of applying it to cmake itself: https://gist.github.com/K900/1da3c1e5e1a46ae1dd5c413f9ec35b1d

[ElvishJerricco]

@K900 Thank you. First we need to fix that particular problem you've mentioned, and then let's try to figure out the impact of a PR like this before we merge such a thing.

[SuperSandro2000]

Given that the sandbox is off by default on Darwin (which seems to be because it doesn't work well enough), this PR would introduce a huge amount of impurities on Darwin. This would be the case not only for users who have Homebrew and/or MacPorts installed alongside Nix, but for any Darwin user, since the system puts files in /usr as well.

To me, this seems like a change that would be a huge detriment to users of Nix on Darwin while providing comparatively little benefit to other Nix users

If this seems to be only useful on darwin, why not make it a darwin only thing and on Linux we just rely on the sandbox?

[Samasaur1]

Given that the sandbox is off by default on Darwin (which seems to be because it doesn't work well enough), this PR would introduce a huge amount of impurities on Darwin. This would be the case not only for users who have Homebrew and/or MacPorts installed alongside Nix, but for any Darwin user, since the system puts files in /usr as well.
To me, this seems like a change that would be a huge detriment to users of Nix on Darwin while providing comparatively little benefit to other Nix users

If this seems to be only useful on darwin, why not make it a darwin only thing and on Linux we just rely on the sandbox?

That seems reasonable to me. And if the Darwin sandbox does reach a point where it is effective and can be enabled by default, this PR could be revisited on Darwin.

[bjornfor]

How will this affect building inside nix-shell on non-NixOS?

[K900]

It will not.

[alexfmpe]

Aaaah so that's what was going on.

changes paths like $out/etc/opt/ to $out/etc/var/empty/

Ran into one of these myself during what would otherwise be a maintenance bump of a package because src/opt is a folder with source files there and paths got rewritten to src/var/empty. A lot of head-scratching later I find this issue and the dontFixCmake = true; fix.

Since this PR seems a ways from being merged, maybe a quick win would be making the sed expression more specific?

nixpkgs/pkgs/by-name/cm/cmake/setup-hook.sh

Line 10 in a292058

sed -e 's^/usr\([ /]\|$\)^/var/empty\1^g' -e 's^/opt\([ /]\|$\)^/var/empty\1^g' < "$fn" > "$fn.tmp"

I'm not sure exactly which characters are allowed in filepaths, but at the very least [a-zA-Z0-9]\+/opt shouldn't be matched right?

[K900]

FWIW I think the way to fix this is to proceed with the approach in #290170. I even got a Hydra jobset for it, but never got around to investigating the failures: https://hydra.nixos.org/jobset/nixpkgs/pr-290170-cmake-hook-shenanigans

[SuperSandro2000]

The build failures there are random failures/time outs.

[K900]

Not all of them, though a lot of them are in fact garbage.

[philiptaron]

@K900 any insights from the above Hydra run?

[K900]

Everything explodes really badly, notably cmake itself. Someone needs to go through cmake and fix it, and then we can continue.

[philiptaron]

@SuperSandro2000, should this then be marked draft or closed? Seems this proposed way forward has some serious thorns.

[SuperSandro2000]

We still want to remove those few lines of shell since they cause really bad side effects but we probably need to fix some of the build failures.

[reckenrode]

but that's also true for any other PATH and those lines of code caused probably more breakages than they fixed and didn't work like expected in the first place anyway.

It is, but the Darwin module bundled with CMake searches those locations by default. It would be nice if it could be made not to do that (even if it doesn’t address every possible problem).

Also aren't the builds run under a different user even outside the sandbox? If that can't write to there, we should be safe, otherwise this seems like a big glaring issue that affects darwin in general.

The concern wasn’t about writing to those locations. It’s that packages installed with other package managers could be picked up instead of those provided by nixpkgs (e.g., enabling unwanted/unexpected features or causing other problems).

This actually is causing problems for the Darwin refactor. I have a workaround unless this lands first, but I wouldn’t mind a patch or fixup to address the default search paths including other package managers on Darwin.

[reckenrode]

After working on #346043 and testing #349555, I would support undoing at least the /usr mangling.

On Darwin, /usr/include doesn’t exist, and /usr/lib barely contains anything. Using the SDK as a sysroot maps those locations to $SDKROOT/usr/include and $SDKROOT/usr/lib, but fixCmakeFiles interferes with that. The .NET build needs to link against Swift frameworks, so it attempts to add -L/usr/lib/swift to the library path, but that gets mangled to -L/var/empty/lib/swift, causing the build to fail to link libswiftFoundation.dylib after #349555.

[emilazy]

I think that we could reasonably specialize it to /usr/(local|X11) without losing anything and fixing a large portion of the problems here.

[emilazy]

After some discussion on Matrix, we came up with the idea that, since -I/foo and -L/foo will look under the SDK root first on Darwin, we could simply ensure that our SDK packages (or a separate stub directory we layer on) contain empty {usr/,opt/}{,local/,X11/}{lib,include} hierarchies. That should ensure that the common compiler tools don’t access anything leaky even with the sandbox off, and allow us to massively reduce the blast radius of getting rid of this hack.