clang: skip the `-nostdlibinc` patch on Darwin; ld64: search standard library locations #349555

emilazy · 2024-10-18T15:19:46Z

It kind of feels like we should find a better way of doing this on Linux too, but I’m picking the conservative option for now. Not tested in any way yet.

Things done

Add a 👍 reaction to pull requests you find important.

alyssais

Shouldn't this be targetPlatform? When cross compiling from Darwin to Linux, we'd want the same behaviour as building natively on Linux, right?

emilazy · 2024-10-18T16:00:46Z

Yyyyes, maybe. Except the reasoning for why it’s safe and won’t cause things like #151879 is based on properties of Darwin as a host platform. So I don’t know, really. Part of me hopes that when cross to Darwin is a thing we’ll have figured out a way to get rid of this patch on Linux too, but I can change it to targetPlatform if you think it’s best, since after all there is no way to hurt yourself with it currently anyway.

emilazy · 2024-10-18T16:02:05Z

Oh, sorry, I misread you as talking about cross the other way around. I think it is safe for cross to Linux. This patch is only here to stop host platform stuff leaking in on non‐NixOS Linux. But it can’t really leak in on macOS because the stuff that would leak in isn’t there. And you’d have a cross sysroot for that case, anyway. But I guess the compiler would behave slightly differently (i.e., better, basically).

paparodeo · 2024-10-18T17:34:32Z

pkgs/development/compilers/llvm/common/clang/default.nix

+    '' + (
+      # See the comment on the `add-nostdlibinc-flag.patch` patch in
+      # `../default.nix` for why we skip Darwin here.
+      if lib.versionOlder release_version "13" && !stdenv.hostPlatform.isDarwin then ''
+        sed -i -e 's/DriverArgs.hasArg(options::OPT_nostdlibinc)/true/' \
+               -e 's/Args.hasArg(options::OPT_nostdlibinc)/true/' \
+               lib/Driver/ToolChains/*.cpp
+      '' else ''
+        (cd tools && ln -s ../../clang-tools-extra extra)
+      ''
+    ) + lib.optionalString stdenv.hostPlatform.isMusl ''


this is removed in #348568 i think.

Yes, 8708697.

It’s replaced with a patch, but I think the logic of not using it on Darwin would remain in that case.

reckenrode · 2024-10-20T12:59:33Z

This is the issue and PR that added the patch. It fixed a build failure on Linux. I tried looking through the history, but this behavior has been around for quite a while. I also assume it’s purity related, but I don’t know.

#151879

#153963

Ericson2314 · 2024-10-20T14:35:23Z

~~The name of the patch seems confusing? It implies is adding a flag, which should be harmless? But then the comment says something else.~~

Oh nevermind adding a flag to the actual command line, not support for a flag

emilazy · 2024-10-21T13:53:51Z

So the situation is:

This reasoning for why this patch is harmful and requires annoying workarounds in downstream packages applies to targeting macOS.
The reasoning for why this patch is mostly doesn’t prevent any brokenness applies to building on macOS.
Theoretically, this change is backwards‐incompatible in very marginal cases where you’re running an unsandboxed build on macOS without any SDK and have Homebrew headers installed. One could construe this as a breaking change to a Release Critical Package, although I think the circumstances in which it would arise are very much not release‐critical.

Ideally, the patch should be applied based on targetPlatform. However, that would expose a hypothetical Linux‐to‐Darwin cross toolchain to regressions of the bugs Randy linked. I don’t know whether we care about that, especially as such toolchains don’t currently exist. Perhaps it could apply when both the host and target platform are Darwin for now to be conservative? I will go with whatever people think is best.

I would like to get this in for 24.11 because currently Rust bindgen stuff is broken on macOS and I don’t think it’s meaningfully backwards‐incompatible in a way that anyone will notice, but pinging @RossComputerGuy just in case.

emilazy · 2024-10-23T16:37:15Z

Pushed to clean up now‐unnecessary hacks in the Darwin stdenv, and apply the same change to ld64 as @reckenrode pointed out that the same issues came up there with things like the .NET build. Will get some builds run to test that I didn’t break anything here.

I also conditionalized the patch on both the host and target platform being Darwin for maximum safety for now. Hopefully in the future we can avoid the sysroot weirdness that makes this necessary on Linux hosts entirely.

RossComputerGuy

LGTM before the 25th.

niklaskorz

zed-editor now builds fine on darwin without the NIX_CFLAGS_COMPILE = "-F${apple-sdk_15.sdkroot}/System/Library/Frameworks"; workaround for rust-bindgen 🥳

┏━ Dependency Graph:
┃    ┌─ ✔ blade-util-0.1.0 ⏱ 1s
┃    ├─ ✔ blade-macros-0.3.0 ⏱ 1s
┃    ├─ ✔ tree-sitter-gowork-0.0.1
┃    ├─ ✔ tree-sitter-heex-0.0.1
┃    ├─ ✔ xim-ctext-0.3.0
┃    ├─ ✔ tree-sitter-md-0.3.2
┃    ├─ ✔ xim-0.4.0
┃    ├─ ✔ xim-parser-0.2.1
┃    ├─ ✔ xkbcommon-0.7.0
┃    ├─ ✔ tree-sitter-yaml-0.6.1 ⏱ 8s
┃ ┌─ ✔ cargo-vendor-dir ⏱ 34s
┃ ├─ ✔ cargo-bundle-unstable-2023-08-18 ⏱ 2m46s
┃ ├─ ✔ cargo-about-0.6.4 ⏱ 3m47s
┃ ✔ zed-editor-0.157.5 ⏱ 41m5s
┣━━━ Builds
┗━ ∑ ⏵ 0 │ ✔ 1903 │ ⏸ 0 │ Finished at 08:30:05 after 11h24m32s

We have to leave the flags in the wrapper until the bootstrap tools are updated, but drop the redundant `-isysroot` to fix issues like <NixOS#349555>.

emilazy · 2024-10-24T08:51:54Z

Zed working confirms that this PR accomplishes its primary goal 🎉 Still waiting to hear about .NET. Swift is being a pain and we might have to back out those changes for now. Testing dropping the redundant -isysroot to fix #350853.

alyssais · 2024-10-24T10:44:04Z

Ack.

This is basically harmless for the same reason as it is for Clang, and lets us avoid doing wrapper hacks to fix things like the .NET build. This reverts commit 4340a5a.

emilazy · 2024-10-24T17:17:12Z

Dropped the Swift changes; it’s too haunted to deal with right now and it works okay without the commit. Moved the flags for backwards‐compatibility with the current bootstrap tools into the stdenv. This should be ready to go pending confirmation that it does in fact build .NET successfully.

emilazy · 2024-10-25T00:02:18Z

Summary of the state of things: it bootstraps fine and fixes the regression, can build Swift, Zed, etc., but the .NET build fails due to Swift‐related ld64 wrapper flag issues that need to be diagnosed; it’s possible that part of the flags wrapper just needs to be restored, at least for now, though it could also theoretically be worked around in the .NET rebuild. I will investigate tomorrow; I hope it is okay for this to miss the deadline by a day since the circumstances in which this is at all a breaking change are very marginal and it will be many days before the staging cycle this lands in even starts anyway.

emilazy · 2024-10-25T00:49:24Z

It turns out that .NET’s build system just has -L/usr/lib/swift references that our awful CMake hook was obliterating and we were inadvertently working around that, and everything about our toolchain is actually working as expected with this PR. We may want to consider automatically adding usr/lib/swift to match the closed‐source ld-prime in the future, but that’s out of scope for this. It’s less than an hour late (in UTC) so hopefully this is okay to merge.

reckenrode · 2024-10-25T00:55:36Z

The stdenv changes LGTM.

nixos-discourse · 2024-11-04T20:44:00Z

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

https://discourse.nixos.org/t/the-darwin-sdks-have-been-updated/55295/26

emilazy requested review from a team October 18, 2024 15:19

github-actions bot added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Oct 18, 2024

nix-owners bot requested review from alyssais and RossComputerGuy October 18, 2024 15:21

emilazy force-pushed the push-smtsootnqmlv branch from 9ee5389 to feb9fd5 Compare October 18, 2024 15:28

alyssais reviewed Oct 18, 2024

View reviewed changes

ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Oct 18, 2024

paparodeo reviewed Oct 18, 2024

View reviewed changes

ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild label Oct 18, 2024

ofborg bot requested review from Ericson2314, rrbutani, sternenseemann, lovek323, alyssais and dtzWill October 18, 2024 18:06

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 18, 2024

niklaskorz mentioned this pull request Oct 20, 2024

zed-editor: fix darwin #329653

Merged

13 tasks

emilazy changed the title ~~clang: skip the -nostdlibinc patch on Darwin~~ clang: skip the -nostdlibinc patch on Darwin; ld64: search standard library locations Oct 23, 2024

emilazy force-pushed the push-smtsootnqmlv branch from feb9fd5 to 49da105 Compare October 23, 2024 16:36

emilazy force-pushed the push-smtsootnqmlv branch from 49da105 to 6784d7e Compare October 23, 2024 16:42

github-actions bot removed the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Oct 23, 2024

emilazy force-pushed the push-smtsootnqmlv branch from 0f0c00b to 8c87bfa Compare October 23, 2024 23:00

RossComputerGuy approved these changes Oct 23, 2024

View reviewed changes

niklaskorz approved these changes Oct 24, 2024

View reviewed changes

emilazy added a commit to emilazy/nixpkgs that referenced this pull request Oct 24, 2024

clang: skip the -nostdlibinc patch on Darwin

4feb218

We have to leave the flags in the wrapper until the bootstrap tools are updated, but drop the redundant `-isysroot` to fix issues like <NixOS#349555>.

emilazy force-pushed the push-smtsootnqmlv branch from 8c87bfa to 37b4e76 Compare October 24, 2024 08:51

emilazy force-pushed the push-smtsootnqmlv branch from 37b4e76 to 3f0333e Compare October 24, 2024 15:54

github-actions bot added the 6.topic: stdenv Standard environment label Oct 24, 2024

nix-owners bot requested a review from toonn October 24, 2024 15:55

clang: skip the -nostdlibinc patch on Darwin

514b00c

emilazy force-pushed the push-smtsootnqmlv branch 2 times, most recently from 6013ee9 to 670a9d2 Compare October 24, 2024 17:04

ld64: search standard library locations

b75c702

This is basically harmless for the same reason as it is for Clang, and lets us avoid doing wrapper hacks to fix things like the .NET build. This reverts commit 4340a5a.

emilazy force-pushed the push-smtsootnqmlv branch from 670a9d2 to b75c702 Compare October 24, 2024 17:12

emilazy changed the title ~~clang: skip the -nostdlibinc patch on Darwin; {ld64,swift}: search standard library locations~~ clang: skip the -nostdlibinc patch on Darwin; ld64: search standard library locations Oct 24, 2024

ofborg bot requested a review from RossComputerGuy October 24, 2024 22:23

emilazy marked this pull request as draft October 25, 2024 00:02

emilazy marked this pull request as ready for review October 25, 2024 00:37

reckenrode mentioned this pull request Oct 25, 2024

cmake: remove fixCmakeFiles #232522

Open

12 tasks

emilazy merged commit 4b8e58e into NixOS:staging Oct 25, 2024
32 of 35 checks passed

emilazy deleted the push-smtsootnqmlv branch October 25, 2024 00:56

emilazy mentioned this pull request Oct 26, 2024

firefox: enable darwin builds; also some of its derivatives (thunderbird, librewolf, floorp) #350384

Merged

13 tasks

niklaskorz mentioned this pull request Oct 28, 2024

zed-editor: remove darwin workaround for isysroot #351973

Merged

13 tasks

Liamolucko mentioned this pull request Dec 9, 2024

isabelle: fix darwin build #348092

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang: skip the `-nostdlibinc` patch on Darwin; ld64: search standard library locations #349555

clang: skip the `-nostdlibinc` patch on Darwin; ld64: search standard library locations #349555

emilazy commented Oct 18, 2024

alyssais left a comment

emilazy commented Oct 18, 2024

emilazy commented Oct 18, 2024

paparodeo Oct 18, 2024 •

edited

Loading

sternenseemann Oct 20, 2024

reckenrode Oct 20, 2024

reckenrode commented Oct 20, 2024 •

edited

Loading

Ericson2314 commented Oct 20, 2024 •

edited

Loading

emilazy commented Oct 21, 2024

emilazy commented Oct 23, 2024 •

edited

Loading

RossComputerGuy left a comment

niklaskorz left a comment

emilazy commented Oct 24, 2024

alyssais commented Oct 24, 2024

emilazy commented Oct 24, 2024

emilazy commented Oct 25, 2024

emilazy commented Oct 25, 2024 •

edited

Loading

reckenrode commented Oct 25, 2024

nixos-discourse commented Nov 4, 2024

clang: skip the -nostdlibinc patch on Darwin; ld64: search standard library locations #349555

clang: skip the -nostdlibinc patch on Darwin; ld64: search standard library locations #349555

Conversation

emilazy commented Oct 18, 2024

Things done

alyssais left a comment

Choose a reason for hiding this comment

emilazy commented Oct 18, 2024

emilazy commented Oct 18, 2024

paparodeo Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

sternenseemann Oct 20, 2024

Choose a reason for hiding this comment

reckenrode Oct 20, 2024

Choose a reason for hiding this comment

reckenrode commented Oct 20, 2024 • edited Loading

Ericson2314 commented Oct 20, 2024 • edited Loading

emilazy commented Oct 21, 2024

emilazy commented Oct 23, 2024 • edited Loading

RossComputerGuy left a comment

Choose a reason for hiding this comment

niklaskorz left a comment

Choose a reason for hiding this comment

emilazy commented Oct 24, 2024

alyssais commented Oct 24, 2024

emilazy commented Oct 24, 2024

emilazy commented Oct 25, 2024

emilazy commented Oct 25, 2024 • edited Loading

reckenrode commented Oct 25, 2024

nixos-discourse commented Nov 4, 2024

clang: skip the `-nostdlibinc` patch on Darwin; ld64: search standard library locations #349555

clang: skip the `-nostdlibinc` patch on Darwin; ld64: search standard library locations #349555

paparodeo Oct 18, 2024 •

edited

Loading

reckenrode commented Oct 20, 2024 •

edited

Loading

Ericson2314 commented Oct 20, 2024 •

edited

Loading

emilazy commented Oct 23, 2024 •

edited

Loading

emilazy commented Oct 25, 2024 •

edited

Loading