curl: fix build failures due to needing to propagate frameworks #263598

reckenrode · 2023-10-26T14:09:18Z

Description of changes

curl expects to propagate any libraries and frameworks it links against, which was missed in #260963. This results in Nix and other packages that have curl as a dependency to fail to build. Since propagating frameworks is not compatible with the current mechanism for overriding the SDK used, a new mechanism has been introduced that handles propagated build inputs. This mechanism will likely replace the existing one in the future.

Things done

reckenrode · 2023-10-26T15:13:22Z

I pushed a change to make it handle paths with spaces. I wanted to use the ASCII record separator, but there’s no way to use hex escapes in Nix, and I didn’t want to put the raw character in the file (since it seemed brittle due to how/whether different editors would treat or display the character).

drupol · 2023-10-26T15:27:17Z

LGTM, I fully trust you on this @reckenrode !

toonn

I'm not sure about the latest attribute. I worry it might be abused and then cause unnecessary rebuilds. I assume you're thinking of cases like MoltenVK but wouldn't you only need an SDK bump for those when the version is bumped?

Overall I support the approach if cURL necessitates it.

toonn · 2023-10-26T16:05:47Z

pkgs/stdenv/adapters.nix

@@ -246,4 +246,106 @@ rec {
        env = (args.env or {}) // { NIX_CFLAGS_COMPILE = toString (args.env.NIX_CFLAGS_COMPILE or "") + " ${toString compilerFlags}"; };
      });
    });
+
+  # Override SDK changes the SDK used to build the package. It does two things:


Suggested change

# Override SDK changes the SDK used to build the package. It does two things:

# Overriding the SDK changes the SDK used to build the package. It does two things:

Alternatively stick to the name of the function, the current form reads a bit weird.

I can clean this and the other comments up.

toonn · 2023-10-26T16:09:40Z

pkgs/stdenv/adapters.nix

+  # * It ensures that the compiler and bintools have the correct Libsystem version; and
+  # * It replaces any SDK references with those in the SDK corresponding to darwinSdkVersion.


Suggested change

# * It ensures that the compiler and bintools have the correct Libsystem version; and

# * It replaces any SDK references with those in the SDK corresponding to darwinSdkVersion.

# * Ensure that the compiler and bintools have the correct Libsystem version

# * Replace any SDK references with those in the SDK corresponding to darwinSdkVersion.

Since the argument isn't necessarily an attrset with the darwinSdkVersion attribute, this reads a bit weird. Maybe "the requested version?"

toonn · 2023-10-26T16:14:46Z

pkgs/stdenv/adapters.nix

+              for targetPath in "$pkgOutputPath"/*; do
+                targetName=$(basename "$targetPath")
+
+                # `nix-support` is special-cased because any propgated inputs need their SDK


Suggested change

# `nix-support` is special-cased because any propgated inputs need their SDK

# `nix-support` is special-cased because any propagated inputs need their SDK

pkgs/stdenv/adapters.nix

toonn · 2023-10-26T16:20:03Z

pkgs/stdenv/adapters.nix

+
+      sdk = pkgs.darwin."apple_sdk_${lib.replaceStrings [ "." ] [ "_" ] darwinSdkVersion}";
+
+      isSDKFramework = pkg: lib.hasPrefix "apple-framework-" (lib.getName pkg);


Should we not do something similar for apple-lib-*? That includes things like XPC.

Eventually. That’s something I’d like to review in conjunction with or after #229210. There’s not a lot of consistency outside of the framework case, and I didn’t want to make the scope of this too broad. That’s why I’m calling it an MVP and keeping it limited to these packages. There’s still a lot of work left before we could conceivably do a tree-wide switch to the adapter over using apple_sdk_11_0.callPackage.

toonn · 2023-10-26T16:25:04Z

pkgs/top-level/all-packages.nix

@@ -5755,7 +5755,9 @@ with pkgs;

  joystickwake = callPackage ../tools/games/joystickwake { };

-  juce = darwin.apple_sdk_11_0.callPackage ../development/misc/juce { };
+  juce = callPackage ../development/misc/juce {
+    stdenv = if stdenv.isDarwin then overrideSDK stdenv "11.0" else stdenv;


Might be worth introducing a stdenv_SDK_11 or such for this. The if-then-else makes it look kind of complicated.

SDK-specific stdenvs don’t compose very well. Even if the adapter is available, people might think you have to use one of those instead of overriding the SDK in the stdenv you want to use. For example, if overrideSDK stdenv is the standard pattern, it’s obvious you should be able to do overrideSDK llvmPackages_15.stdenv "11.0" or overrideSDK qt6Packages.stdenv "11.0" or overrideSDK python3.stdenv "11.0" or even overrideSDK python3.stdenv { darwinMinVersion = "10.13";}.

The last one is equivalent to what I had to do manually to fix pybind11 for clang 16. There are a couple of other packages that have to do that as well. It would be much nicer if they could just use the adapter to set the deployment target in the stdenv.

Regarding the conditional, you’d need it even if there were a SDK-specific stdenv. I’ve intentionally chosen not to make the adapter platform-neutral. It’s for Darwin, so you need to conditionally use it only on Darwin. I’m pretty sure I’ve seen it mentioned that the current approach is a layering violation, and I agree. This PR fixes that by making the adapter work like other platform-specific stuff in nixpkgs.

reckenrode · 2023-10-26T17:09:02Z

@toonn I pushed an update that fixes the comments, cleans up the code a bit, and drops support for specifying the latest SDK. You’re right I was thinking of MoltenVK. I can handle that manually as necessary.

toonn

Only another typo. LGTM

pkgs/stdenv/adapters.nix

toonn · 2023-10-26T17:13:47Z

pkgs/stdenv/adapters.nix

@@ -246,4 +246,103 @@ rec {
        env = (args.env or {}) // { NIX_CFLAGS_COMPILE = toString (args.env.NIX_CFLAGS_COMPILE or "") + " ${toString compilerFlags}"; };
      });
    });
+
+  # Overriding the SDK changes the Darwin SDK used to build the package, which:
+  # * Ensures that the compiler and bintools have the correct Libsystem version; and


The "; and" feels very awkward in a bulleted list, IMHO. Not important though.

It’s a style thing. I try to write bulleted lists as if they were part of the sentence that precedes them (but laid out as lists for readability). But I can drop it if that’s the preference.

toonn · 2023-10-26T17:15:41Z

pkgs/stdenv/adapters.nix

+  # * Replaces any SDK references with those in the SDK corresponding to requested SDK version.
+  #
+  # `sdkVersion` can be any of the following:
+  # * A version string indicating the requested SDK version; or


If the "; and" from before is dropped the "; or" here needs the same treatment.

This is a replacement for using `darwin.apple_sdk_<ver>.callPackage`. Instead of injecting the required packages, it provides a stdenv adapter that modifies the derivation’s build inputs to use the requested SDK versions. This modification extends to any build inputs propagated to it as well. The `callPackage` approach is not deprecated yet, but it is expected that it will be eventually. Note that this is an MVP. It should work with most packages, but it only handles build inputs and also only handles frameworks. Once more SDKs are added (after NixOS#229210 is merged) and the SDK structure is normalized, it can be extended to handle any package in the SDK namespace. Cross-compilation may or may not work. Any cross-related issues can be addressed after NixOS#256590 is merged.

Using overrideSDK allows juce to use the correct SDK frameworks even when they are propagated from curl.

Using overrideSDK allows datadog_trace to use the correct SDK frameworks even when they are propagated from curl.

Using overrideSDK allows cargo-codspeed to use the correct SDK frameworks even when they are propagated from curl.

`libcurl.la` and `curl-config --static-libs` both include the frameworks curl needs to link, so propgate them in case dependents don’t include those frameworks in their build inputs. This fixes building Nix and probably other things (like netcdf).

reckenrode · 2023-10-26T17:29:13Z

I pushed a fix for the typo. I left the lists as they were unless I get feedback requesting otherwise.

vcunat · 2023-10-27T05:43:17Z

FYI, the builds happening for several hours on Hydra already use the first version of this PR, but there won't be that much done yet today, so trashing those builds won't be a real issue.

reckenrode · 2023-10-27T11:54:28Z

FYI, the builds happening for several hours on Hydra already use the first version of this PR, but there won't be that much done yet today, so trashing those builds won't be a real issue.

The changes have been in the adapter, which affects three packages. The core change of propagating the build inputs from curl hasn’t changed, so unless something happens to drastically change or drop 028534b, there shouldn’t be much in the way of rebuilds once this gets merged.

toonn

LGTM

Nix built for me with this PR.

reckenrode requested review from jtojnar, aanderse, drupol, etu, globin, Ma27 and talyz as code owners October 26, 2023 14:09

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

reckenrode mentioned this pull request Oct 26, 2023

Discussion: The future of SDKs on Darwin #242666

Closed

reckenrode force-pushed the curl-propagation-fix branch from 54328f0 to 93fcc5f Compare October 26, 2023 15:11

toonn reviewed Oct 26, 2023

View reviewed changes

reckenrode force-pushed the curl-propagation-fix branch from 93fcc5f to 2864180 Compare October 26, 2023 17:08

toonn reviewed Oct 26, 2023

View reviewed changes

reckenrode added 5 commits October 26, 2023 13:26

juce: switch to overrideSDK

19214a2

Using overrideSDK allows juce to use the correct SDK frameworks even when they are propagated from curl.

phpExtensions.datadog_trace: switch to overrideSDK

c2e00b1

Using overrideSDK allows datadog_trace to use the correct SDK frameworks even when they are propagated from curl.

cargo-codspeed: switch to overrideSDK

4239c39

Using overrideSDK allows cargo-codspeed to use the correct SDK frameworks even when they are propagated from curl.

curl: propagate Darwin frameworks

028534b

`libcurl.la` and `curl-config --static-libs` both include the frameworks curl needs to link, so propgate them in case dependents don’t include those frameworks in their build inputs. This fixes building Nix and probably other things (like netcdf).

reckenrode force-pushed the curl-propagation-fix branch from 2864180 to 028534b Compare October 26, 2023 17:26

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

ofborg bot requested review from lovek323, kashw2, adevress, figsoda and prusnak October 26, 2023 19:59

ofborg bot requested review from SuperSandro2000 and AndersonTorres October 26, 2023 19:59

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 26, 2023

toonn approved these changes Oct 27, 2023

View reviewed changes

toonn merged commit f65ccb3 into NixOS:staging-next Oct 27, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

curl: fix build failures due to needing to propagate frameworks #263598

curl: fix build failures due to needing to propagate frameworks #263598

reckenrode commented Oct 26, 2023

reckenrode commented Oct 26, 2023

drupol commented Oct 26, 2023

toonn left a comment

toonn Oct 26, 2023

reckenrode Oct 26, 2023

toonn Oct 26, 2023

toonn Oct 26, 2023

toonn Oct 26, 2023

reckenrode Oct 26, 2023

toonn Oct 26, 2023

reckenrode Oct 26, 2023 •

edited

Loading

reckenrode commented Oct 26, 2023 •

edited

Loading

toonn left a comment

toonn Oct 26, 2023

reckenrode Oct 26, 2023 •

edited

Loading

toonn Oct 26, 2023

reckenrode commented Oct 26, 2023

vcunat commented Oct 27, 2023

reckenrode commented Oct 27, 2023

toonn left a comment

	# Override SDK changes the SDK used to build the package. It does two things:
	# Overriding the SDK changes the SDK used to build the package. It does two things:

		# * It ensures that the compiler and bintools have the correct Libsystem version; and
		# * It replaces any SDK references with those in the SDK corresponding to darwinSdkVersion.

	# `nix-support` is special-cased because any propgated inputs need their SDK
	# `nix-support` is special-cased because any propagated inputs need their SDK


		sdk = pkgs.darwin."apple_sdk_${lib.replaceStrings [ "." ] [ "_" ] darwinSdkVersion}";

		isSDKFramework = pkg: lib.hasPrefix "apple-framework-" (lib.getName pkg);

curl: fix build failures due to needing to propagate frameworks #263598

curl: fix build failures due to needing to propagate frameworks #263598

Conversation

reckenrode commented Oct 26, 2023

Description of changes

Things done

reckenrode commented Oct 26, 2023

drupol commented Oct 26, 2023

toonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckenrode Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

reckenrode commented Oct 26, 2023 • edited Loading

toonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckenrode Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckenrode commented Oct 26, 2023

vcunat commented Oct 27, 2023

reckenrode commented Oct 27, 2023

toonn left a comment

Choose a reason for hiding this comment

reckenrode Oct 26, 2023 •

edited

Loading

reckenrode commented Oct 26, 2023 •

edited

Loading

reckenrode Oct 26, 2023 •

edited

Loading