macos: change rules and improve detection of native SDK #16212

kubkon · 2023-06-25T20:25:43Z

Summary of breaking changes:

if we build on an Apple host (mainly macOS) and we target an Apple platform, we will try linking against the SDK if available
however, if we detect Nix, we ignore native SDK and instead rely fully on paths supplied by Nix via env vars

Now, the nitty-gritty:
In order to achieve 1. we have to apply this rule to all possible Compilation.create invocations which made it natural to move some of the existing code that is concerned with native paths detection into Compilation.create. And so, now detection of native Apple SDK is done within Compilation.create, as well as detection/unpacking of paths supplied by Nix. I think this bit of functionality really belongs in LibcInstallation struct, as it is a natural companion to autodetection logic of libc include paths that LibcInstallation provides. I haven't acted on this yet in this PR, so feel free to comment on the correctness of my assertion.

Detection of native libc include paths for Apple platforms is now done via LibcInstallation struct as it is done for other platforms - this got rid of a lot of special casing for Apple in Compilation.zig which is nice. The detection logic in LibcInstallation for Apple platforms relies on a two-tier logic: if SDK was detected, we use that, otherwise, we fallback to clang for detection. The latter is to mainly support the Nix environment. Anyhow, with this PR, zig libc now works on macOS (no more panicking). Instead we get this if the SDK is available:

# The directory that contains `stdlib.h`.
# On POSIX-like systems, include directories be found with: `cc -E -Wp,-v -xc /dev/null`
include_dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include

# The system-specific include directory. May be the same as `include_dir`.
# On Windows it's the directory that includes `vcruntime.h`.
# On POSIX it's the directory that includes `sys/errno.h`.
sys_include_dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include

# The directory that contains `crt1.o` or `crt2.o`.
# On POSIX, can be found with `cc -print-file-name=crt1.o`.
# Not needed when targeting MacOS.
crt_dir=

# The directory that contains `vcruntime.lib`.
# Only needed when targeting MSVC on Windows.
msvc_lib_dir=

# The directory that contains `kernel32.lib`.
# Only needed when targeting MSVC on Windows.
kernel32_lib_dir=

# The directory that contains `crtbeginS.o` and `crtendS.o`
# Only needed when targeting Haiku.
gcc_dir=

# The directory that contains system frameworks (Darwin only).
framework_dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/System/Library/Frameworks

Closes #15024
Closes #14569

EDIT:
Closes #11631

@haze marking #11631 as closed but I will ask you to verify it did indeed completely fix the issue for you once 0.11 lands, and is available via Nix.

EDIT2:
Closes #16118
Closes #15963

haze · 2023-06-26T02:20:52Z

This should also close #11631 no?

andrewrk

Looks great!

I see a couple more things that could be further improved, let me know what you think.

andrewrk · 2023-06-26T18:44:00Z

src/Compilation.zig

+            try clang_argv.ensureUnusedCapacity(paths.include_dirs.items.len * 2);
+            for (paths.include_dirs.items) |include_dir| {
+                clang_argv.appendAssumeCapacity("-isystem");
+                clang_argv.appendAssumeCapacity(include_dir);
+            }
+
+            try clang_argv.ensureUnusedCapacity(paths.framework_dirs.items.len * 2);
+            try framework_dirs.ensureUnusedCapacity(paths.framework_dirs.items.len);
+            for (paths.framework_dirs.items) |framework_dir| {
+                clang_argv.appendAssumeCapacity("-iframework");
+                clang_argv.appendAssumeCapacity(framework_dir);
+                framework_dirs.appendAssumeCapacity(framework_dir);
+            }


The logic pertaining to clang_argv should go in addCCArgs.

andrewrk · 2023-06-26T18:45:45Z

src/Compilation.zig

+        var clang_argv = std.ArrayList([]const u8).init(arena);
+        var lib_dirs = std.ArrayList([]const u8).init(arena);
+        var framework_dirs = std.ArrayList([]const u8).init(arena);
+        var rpath_list = std.ArrayList([]const u8).init(arena);


Can you try adding a commit that adds the needed state here (except clang_argv) to LibCDirs and moving the corresponding logic to detectLibCIncludeDirs or getZigShippedLibCIncludeDirsDarwin?

I think that might clean this up even more.

kubkon · 2023-06-28T08:38:03Z

@andrewrk I took your suggestions a step further and now moved all of the logic into detectLibCFromLibCInstallation which cleaned it all up even more. Have a look and lemme know what you think!

andrewrk

Getting closer. I still see some things where they don't belong. Apologies for the vague review feedback - hopefully knowing what not to do reduces the possibility space so it becomes more clear how to proceed.

andrewrk · 2023-07-01T18:21:29Z

src/Compilation.zig

            break :blk false;
        };

+        const darwin_native = (comptime builtin.target.isDarwin()) and options.target.isDarwin() and


I believe this boolean is already provided via options.is_native_os. It does not currently to take into account sysroot, however, and I'm not sure whether it should. Perhaps this would be better:

Suggested change

const darwin_native = (comptime builtin.target.isDarwin()) and options.target.isDarwin() and

const darwin_native = options.is_native_os and options.target.isDarwin() and

andrewrk · 2023-07-01T19:44:54Z

src/Compilation.zig

+        var lib_dirs = std.ArrayList([]const u8).init(arena);
+        try lib_dirs.appendSlice(options.lib_dirs);
+        try lib_dirs.appendSlice(libc_dirs.libc_lib_dir_list);
+
+        var framework_dirs = std.ArrayList([]const u8).init(arena);
+        try framework_dirs.appendSlice(options.framework_dirs);
+        try framework_dirs.appendSlice(libc_dirs.libc_framework_dir_list);
+
+        var rpath_list = std.ArrayList([]const u8).init(arena);
+        try rpath_list.appendSlice(options.rpath_list);
+        try rpath_list.appendSlice(libc_dirs.libc_rpath_list);


I don't think these should be added here. This information is available to whatever logic needs it via link.File.options.libc_installation

Hmm, all are needed by the linker, and framework paths are needed by the frontend (compiler) too. How else would you provide this info then?

Both the linker and the frontend have access to link.File.options.libc_installation.

Oh, did I finally understand it that you mean we can recreate this info on request in whatever linker needs it?

yes I believe that is correct 👍

Took me long enough to get it...

andrewrk · 2023-07-01T19:47:01Z

src/Compilation.zig

    }

+    if (want_native_paths) {
+        const paths = std.zig.system.NativePaths.detect(arena, target) catch |err| {


This function should not be called from detectLibCFromLibCInstallation. This function is intended to only extract libc information from the LibCInstallation and not inspect the host at all.

kubkon · 2023-07-01T20:07:05Z

I'm not sure how to proceed from here for one reason: our libc detection mechanism so far assumed that libc is only a set of include dirs which is wrong. Depending on the host, in this case macos or any Apple platform, the linker needs also the knowledge of libc search dir paths, rpaths and framework dirs. All of those together with a set of include dirs make up an actually libc installation on a given host. With that in mind, I'm not sure how to go about addressing some of your feedback as it seems to be still based on the assumption that libc installation is only a set of include dirs. Apologies if it is not but it's difficult for me to work out exactly an action plan based on your latest feedback as it's a little too vague. Could you therefore clarify your feedback a bit more?

andrewrk · 2023-07-02T22:57:35Z

Can you make sure that this solves #11066?

Regarding your question above - LibCInstallation is not only headers. Let's look at an example:

# The directory that contains `stdlib.h`.
# On POSIX-like systems, include directories be found with: `cc -E -Wp,-v -xc /dev/null`
include_dir=/nix/store/iczlqwdj10xiz3mmms6adnra0mn3ljk1-glibc-2.35-224-dev/include

# The system-specific include directory. May be the same as `include_dir`.
# On Windows it's the directory that includes `vcruntime.h`.
# On POSIX it's the directory that includes `sys/errno.h`.
sys_include_dir=/nix/store/iczlqwdj10xiz3mmms6adnra0mn3ljk1-glibc-2.35-224-dev/include

# The directory that contains `crt1.o` or `crt2.o`.
# On POSIX, can be found with `cc -print-file-name=crt1.o`.
# Not needed when targeting MacOS.
crt_dir=/nix/store/vnwdak3n1w2jjil119j65k8mw1z23p84-glibc-2.35-224/lib

# The directory that contains `vcruntime.lib`.
# Only needed when targeting MSVC on Windows.
msvc_lib_dir=

# The directory that contains `kernel32.lib`.
# Only needed when targeting MSVC on Windows.
kernel32_lib_dir=

# The directory that contains `crtbeginS.o` and `crtendS.o`
# Only needed when targeting Haiku.
gcc_dir=

As you can see, only 2 out of 6 fields are include directories. Actually most of the fields here are not related to headers.

Unless we detect Nix, in which case, we use Nix provided paths via env vars.

kubkon · 2023-07-28T17:24:52Z

@andrewrk I haven't put the contents of NativePaths into LibCDirs, instead, I have made it an optional in link.File.Options for the linkers to have access to. Lemme know if you think this is heading in the right direction, and/or what else needs adjusting, etc.

…e_os" This reverts commit 00854bd.

andrewrk

I think it needs more work before it's ready to land.

andrewrk · 2023-07-31T17:26:10Z

lib/std/zig/system/NativePaths.zig

+pub inline fn getIncludeDirs(self: *const NativePaths) []const [:0]const u8 {
+    return self.include_dirs.items;
+}
+
+pub inline fn getLibDirs(self: *const NativePaths) []const [:0]const u8 {
+    return self.lib_dirs.items;
+}
+
+pub inline fn getFrameworkDirs(self: *const NativePaths) []const [:0]const u8 {
+    return self.framework_dirs.items;
+}
+
+pub inline fn getRpaths(self: *const NativePaths) []const [:0]const u8 {
+    return self.rpaths.items;
+}
+


why inline?

why do these getters even exist?

It made sense to me instead of typing paths.lib_dirs.items everywhere but it's a matter of preference so I am happy to remove it.

andrewrk · 2023-07-31T17:34:20Z

src/Compilation.zig

-                break :blk sdk.path;
-            } else {
-                break :blk null;
+        const darwin_native = (comptime builtin.target.isDarwin()) and options.target.isDarwin() and


This is incorrect logic for darwin_native. When options.is_native_os is false, darwin_native must be false. Otherwise, it's going to look at the system sometimes when it should be cross-compiling. Zig should not rely on the Darwin SDK when cross-compiling.

Ah, fair point. The reason I re-introduced comptime check is because it then emits darwin-specific code that requires a host that spawn subprocesses which isn't always the case plus it should even exist unless it's a native OS AND Darwin. Can I make it more verbose and add is_native_os here but leave the comptime check? Or perhaps you have a nicer suggestion for this?

andrewrk · 2023-07-31T17:39:35Z

src/Compilation.zig

+            for (paths.warnings.items) |warning| {
+                log.warn("{s}", .{warning});


can you give an example of what this looks like when it occurs?

I cannot, I copied the code that was already there. You may either ask the original author, or we can remove it.

andrewrk · 2023-07-31T17:45:30Z

src/link.zig

+    // TODO figure out if we can make it part of LibCInstallation
+    native_paths: ?NativePaths = null,


I don't think this should be a TODO. I think it should be done as part of this patch.

Sure, but I will require assistance here as I don't yet know how to do that.

andrewrk · 2023-08-03T02:58:52Z

I solved these problems by expanding our existing frontend abstractions that are already designed to handle this situation (and already do handle it on the other operating systems besides macOS). Namely, std.zig.system.NativePaths and LibCInstallation. I made those changes in #16058. Please test that branch and suggest any enhancements there.

kubkon requested a review from andrewrk June 25, 2023 20:25

kubkon mentioned this pull request Jun 25, 2023

macos: try auto-detecting the SDK when building on Apple for Apple #16112

Closed

kubkon added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Jun 25, 2023

kubkon marked this pull request as draft June 25, 2023 21:12

kubkon force-pushed the macos-autodetect-sdk-tmp branch from e322dc4 to ce35ae8 Compare June 26, 2023 06:59

kubkon marked this pull request as ready for review June 26, 2023 09:43

andrewrk requested changes Jun 26, 2023

View reviewed changes

kubkon requested a review from andrewrk June 28, 2023 08:38

This was referenced Jun 28, 2023

zig cc failing to locate framework headers #16235

Open

zig clang failing to locate stdlib headers #16236

Closed

macOS 14 Sonoma tracking issue #15963

Closed

andrewrk requested changes Jul 1, 2023

View reviewed changes

kubkon mentioned this pull request Jul 25, 2023

Fix Two Small build.zig Issues kubkon/zig-ios-example#10

Closed

kubkon added 8 commits July 26, 2023 15:11

macos: try auto-detecting the SDK when building on Apple for Apple

a466bdc

Unless we detect Nix, in which case, we use Nix provided paths via env vars.

macos: find include dir to frameworks using clang as fallback

91c3373

libc: remove unused error in libc_installation.zig

aece56e

main: fix missing type annotation

b48c087

std: make NativePaths.isNix() error out on WASI with no libc

03b7096

comp: move native include dirs detection to addCCArgs

8dbddb9

comp: move native libc dirs detection to detectLibCDirs

7774af9

comp: move all of the logic into detectLibCFromLibCInstallation

6d0f831

kubkon force-pushed the macos-autodetect-sdk-tmp branch from 05eb30e to 6d0f831 Compare July 26, 2023 13:11

kubkon added 3 commits July 27, 2023 21:53

wip: move fetching native paths to spots which actually need them

adc6191

wip: store fetched NativePaths as part of link.File.Options

65d1f4a

wip: replace comptime builtin.target.isDarwin() with is_native_os

00854bd

kubkon requested a review from andrewrk July 28, 2023 17:20

kubkon added 4 commits July 28, 2023 20:18

wip: fix isNix on wasm32-wasi target (with no libc)

9d2090c

std: fix error set of std.process.hasEnvVar

3fd338f

std: fix StackFallbackAllocator usage in std.process.hasEnvVar

ac7ad6c

Revert "wip: replace comptime builtin.target.isDarwin() with is_nativ…

7e1b439

…e_os" This reverts commit 00854bd.

andrewrk requested changes Jul 31, 2023

View reviewed changes

andrewrk closed this Aug 3, 2023

kubkon deleted the macos-autodetect-sdk-tmp branch September 1, 2023 12:38

	const darwin_native = (comptime builtin.target.isDarwin()) and options.target.isDarwin() and
	const darwin_native = options.is_native_os and options.target.isDarwin() and

		for (paths.warnings.items) \|warning\| {
		log.warn("{s}", .{warning});

		// TODO figure out if we can make it part of LibCInstallation
		native_paths: ?NativePaths = null,

Uh oh!

macos: change rules and improve detection of native SDK #16212

macos: change rules and improve detection of native SDK #16212

Uh oh!

Conversation

kubkon commented Jun 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haze commented Jun 26, 2023

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

andrewrk Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented Jun 28, 2023

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented Jul 1, 2023

Uh oh!

andrewrk commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubkon commented Jul 28, 2023

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrk commented Aug 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kubkon commented Jun 25, 2023 •

edited

Loading

andrewrk Jun 26, 2023 •

edited

Loading

andrewrk commented Jul 2, 2023 •

edited

Loading