Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Darwin platform versions #111988

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

thefloweringash
Copy link
Member

@thefloweringash thefloweringash commented Feb 5, 2021

Motivation for this change

Implement the reproducibility of #77632, without the warnings that were introduced by cctools (#93596 (comment)), with support for cross compilation by moving sdk versions and deployment targets out of the darwin stdenv and onto targetPlatform.

Effectively reverts: #110140 by @holymonson. When working on aarch64-darwin the SDK version provided (and embedded in LC_BUILD_VERSION) controls some kind of compatibility layer, see gist, so I think we need to support an idea of "SDK version" even though nixpkgs generally treats darwin as a set of independent and overridable packages.

Also tries to define a platform definition structure for all darwin targets including ios. Currently these have platforms have equivalent attributes which I would like to unify with the rest of darwin. The extra configuration here pkgs/os-specific/darwin/xcode/sdk-pkgs.nix should be able to entirely removed, and instead be provided by the standard darwin stdenv and binutils.

Original PR: #77632
Related: #103053, #83180
Motivation: #105026 (comment)

Tested:

Untested:

  • ios cross

Motivating test case

The following set of test cases should include no information from the building host and produce no linker warnings.

See results in $(nix-build version-test-case.nix)/summary.

version-test-case.nix
let
  pkgs = import ./. {};
  inherit (pkgs) lib;

  ## Default stdenv
  stdenv = pkgs.stdenv;

  ## With clang 11
  # stdenv = pkgs.overrideCC pkgs.stdenv pkgs.buildPackages.clang_11;

  ## With clang 10
  # stdenv = pkgs.overrideCC pkgs.stdenv pkgs.buildPackages.clang_10;

  source = pkgs.writeText "main.c" ''
    int main() {}
  '';

  tests = {
    clangDefault = ''
      $CC -v -o test ${source}
    '';

    clangExplicit = ''
      $CC -v -mmacos-version-min=10.14 -o test ${source}
    '';

    ldDefault = ''
      $CC -o test.o -c ${source}
      $LD -L${pkgs.darwin.Libsystem}/lib -lSystem -o test test.o
    '';

    ldMacosVersion = ''
      $CC -o test.o -c ${source}
      $LD \
        -macos_version_min 10.14 \
        -L${pkgs.darwin.Libsystem}/lib -lSystem -o test test.o
    '';

    ldBothLegacyFlags = ''
      $CC -o test.o -c ${source}
      $LD \
        -macos_version_min 10.14 \
        -sdk_version 10.14 \
        -L${pkgs.darwin.Libsystem}/lib -lSystem -o test test.o
    '';

    ldSdkVersion = ''
      $CC -o test.o -c ${source}
      $LD \
        -sdk_version 10.14 \
        -L${pkgs.darwin.Libsystem}/lib -lSystem -o test test.o
    '';

    ldPlatformVersion = ''
      $CC -o test.o -c ${source}
      $LD \
        -platform_version macos 10.14 10.14 \
        -L${pkgs.darwin.Libsystem}/lib -lSystem -o test test.o
    '';
  };

  buildTest = name: testScript: stdenv.mkDerivation {
    inherit name testScript;
    passAsFile = [ "testScript" "buildCommand" ];

    buildCommand = ''
      set -u
      echo $testScriptPath
      mkdir $out
      NIX_DEBUG=3 $SHELL -x $testScriptPath 2>&1 | tee $out/build.log
      ${pkgs.darwin.cctools}/bin/otool -l test> $out/load_commands
    '';
  };
in
  pkgs.runCommand "all-tests" {
    passthru = lib.mapAttrs buildTest tests;
  } (
    ''
      mkdir $out
    '' +
    lib.concatStringsSep "\n" (
      lib.mapAttrsToList (name: script: ''
        ln -s ${buildTest name script} $out/${name}
      '') tests
    ) + ''
      cd $out
      {
        showCommand() {
          local cmdName=$1
          echo -ne "\t$cmdName: "
          awk "/$cmdName/ { show = 1 }; /Load command/ { show = 0 }; show" < $i | tr '\n' '\t'
          echo
        }

        for i in */load_commands; do
          echo $i
          showCommand LC_BUILD_VERSION
          showCommand LC_UUID
          showCommand LC_VERSION_MIN_MACOSX
        done

        echo

        echo "Linker warnings:"
        grep -nH 'warning' */build.log || true
      } > summary 2>&1
    ''
  )

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Feb 5, 2021
@thefloweringash thefloweringash marked this pull request as draft February 5, 2021 07:28
@ofborg ofborg bot added the 6.topic: stdenv Standard environment label Feb 5, 2021
@holymonson
Copy link
Contributor

FYI, I was working on #111658 which could support multiple versions of sdk (by generating several info lists). I'm not sure if you need it, but I will pause here.

Comment on lines 23 to 24
postPatch = lib.optionalString stdenv.hostPlatform.isDarwin ''
substituteInPlace src/luarocks/core/cfg.lua --subst-var-by 'darwinMinVersion' '${stdenv.hostPlatform.darwinMinVersion}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
postPatch = lib.optionalString stdenv.hostPlatform.isDarwin ''
substituteInPlace src/luarocks/core/cfg.lua --subst-var-by 'darwinMinVersion' '${stdenv.hostPlatform.darwinMinVersion}'
postPatch = lib.optionalString stdenv.targetPlatform.isDarwin ''
substituteInPlace src/luarocks/core/cfg.lua --subst-var-by 'darwinMinVersion' '${stdenv.targetPlatform.darwinMinVersion}'

because we should do this for other -> darwin cross too, I believe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if nixpkgs has has working cross compilation for lua packages, but assuming it does, targetPackages seems correct here 👍.

@thefloweringash thefloweringash force-pushed the darwin-platform-versions branch from 45862be to ba31b0f Compare February 6, 2021 06:34
@thefloweringash
Copy link
Member Author

With the debugging commit the evaluation error is visible:

darwinPlatform: unknown platform for x86_64-unknown-linux-gnu

Is there an idiom for platform configuration that doesn't apply to all platforms?

@thefloweringash
Copy link
Member Author

This ensures MACOSX_VERSION_MIN is set for the correct platform, but now I'm finding a significant downside. As is, x86_64-darwin uses clang 7 and aarch64-darwin uses clang 11. When cross-compiling for aarch64-darwin, MACOSX_DEPLOYMENT_TARGET=11.0. When clang 7 builds for the build platform it uses this environment variable. It's incorrect since it's for the wrong platform, and it's also an error since clang prior to 11 rejects values not starting with 10. It's easy to work around with NIX_CFLAGS_COMPILE_FOR_BUILD in a given package, but I'm considering adding -mmacos-version-min to cflags in the cc-wrapper.

@thefloweringash thefloweringash force-pushed the darwin-platform-versions branch from ff8e80e to dcfd17a Compare February 24, 2021 01:47
@thefloweringash thefloweringash marked this pull request as ready for review March 2, 2021 07:44
@thefloweringash
Copy link
Member Author

This is ready for review. Still outstanding: how to represent the absence of value for darwin* flags on platforms that aren't darwin.

@veprbl
Copy link
Member

veprbl commented Mar 24, 2021

@thefloweringash This does not evaluate btw.

@veprbl veprbl marked this pull request as draft March 24, 2021 16:11
@thefloweringash thefloweringash force-pushed the darwin-platform-versions branch from da49f21 to b3b3905 Compare March 26, 2021 06:10
@thefloweringash
Copy link
Member Author

@thefloweringash This does not evaluate btw.

I didn't know a nice way to have the darwinPlatform be absent on platforms other than darwin. I had it as a throw which was triggering on linux platforms. I've made it a null, which doesn't quite seem correct but should at least not break unless it's actually used, and not silently fail if interpolated.

@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild labels Mar 26, 2021
@veprbl veprbl marked this pull request as ready for review March 26, 2021 14:02
@Ericson2314
Copy link
Member

null seems fine to me.

@Ericson2314
Copy link
Member

@thefloweringash Are you going to change https://github.com/NixOS/nixpkgs/blob/master/pkgs/os-specific/darwin/xcode/sdk-pkgs.nix later then?

Also, was the MACOSX_DEPLOYMENT_TARGET issues resolved? This sounds like the sort of thing where we could do MACOSX_DEPLOYMENT_TARGET and MACOSX_DEPLOYMENT_TARGET_FOR_BUILD, and the build compiler would the "suffix salt" black magic to get the right one. (c.f. the pkg-config wrapper and PKG_CONFIG_PATH for the most self-contained example of that.)

@thefloweringash thefloweringash force-pushed the darwin-platform-versions branch 2 times, most recently from 914dcdc to 672eb33 Compare April 10, 2021 09:28
Instead of always supplying flags, apply the flags as defaults. Use
clang's native flags instead of lifting the linker flags from binutils
with `-Wl,`.

If a project is using clang to drive linking, make clang do the right
thing with MACOSX_DEPLOYMENT_TARGET. This can be overridden by command
line arguments. This will cause modern clang to pass
`-platform_version 10.12 0.0.0`, since it doesn't know about the SDK
settings. Older versions of clang will pass down `-macos_version_min`
flags with no sdk version.

At the linker layer, apply a default value for anything left
ambiguous. If nothing is specified, pass a full
`-platform_version`. If only `-macos_version_min` is specified, then
lock down the sdk_version explicitly with `-sdk_version`. If a min
version and sdk version is passed, do nothing.
This avoids contamination via MACSOX_DEPLOYMENT_TARGET when cross
compiling.
…oles

In a typical build environment the toolchain will use the value of the
MACOSX_DEPLOYMENT_TARGET environment variable to determine the version
of macOS to support. When cross compiling there are two distinct
toolchains, but they will look at this single environment variable. To
avoid contamination, we always set the equivalent command line flag
which effectively disables the toolchain's internal handling.

Prior to this change, the MACOSX_DEPLOYMENT_TARGET variable was
ignored, and the toolchains always used the Nix platform
definition (`darwinMinVersion`) unless overridden with command line
arguments.

This change restores support for MACOSX_DEPLOYMENT_TARGET, and adds
nix-specific MACOSX_DEPLOYMENT_TARGET_FOR_BUILD and
MACOSX_DEPLOYMENT_TARGET_FOR_TARGET for cross compilation.
These are now provided by the standard bintools and cc wrappers.
@thefloweringash thefloweringash force-pushed the darwin-platform-versions branch from 672eb33 to 33265e0 Compare April 11, 2021 00:47
@Ericson2314
Copy link
Member

Is this ready now?

@thefloweringash
Copy link
Member Author

@thefloweringash Are you going to change https://github.com/NixOS/nixpkgs/blob/master/pkgs/os-specific/darwin/xcode/sdk-pkgs.nix later then?

Updated to include this. I don't know of any example that uses it in the nixpkgs tree so I haven't tested it. If anyone that relies on cross compiling to iPhone has a chance to test it, it would be appreciated.

Also, was the MACOSX_DEPLOYMENT_TARGET issues resolved? This sounds like the sort of thing where we could do MACOSX_DEPLOYMENT_TARGET and MACOSX_DEPLOYMENT_TARGET_FOR_BUILD, and the build compiler would the "suffix salt" black magic to get the right one. (c.f. the pkg-config wrapper and PKG_CONFIG_PATH for the most self-contained example of that.)

I've now added support for the MACOSX_DEPLOYMENT_TARGET_FOR_BUILD variable, which required making a variable mangler that operated on a singular value.

Is this ready now?

I believe so.

@domenkozar
Copy link
Member

@Ericson2314 feel free to merge if this looks good to you.

@Ericson2314
Copy link
Member

Took me a second to figure out what mangleVarSingle is for, but yeah looks good. Sometimes I wish we had build-cc, cc, and target-cc rather than these tools prefixed with concrete platforms, and then we wouldn't have to worry about conflicts. Thanks for handling the conflict case correctly.

@Ericson2314 Ericson2314 merged commit 82ca81c into NixOS:staging Apr 12, 2021
@Ericson2314
Copy link
Member

(As to iOS cross, I will someday need to bump our stuff to use this version of Nixpkgs, but i don't have the phones to test with me. I like this design better so if there are regressions they should be plenty easy to fix and totally worth it :).)

@thefloweringash thefloweringash deleted the darwin-platform-versions branch April 13, 2021 01:52
Ericson2314 added a commit to Ericson2314/nixpkgs that referenced this pull request May 6, 2021
I am taking the non-invasive parts of NixOS#110914 to hopefully help out with NixOS#111988.

In particular:

 - Use `lib.makeScopeWithSplicing` to make the `darwin` package set have
   a proper `callPackage`.

 - Adjust Darwin `stdenv`'s overlays keeping things from the previous
   stage to not stick around too much.

 - Expose `binutilsNoLibc` / `darwin.binutilsNoLibc` to hopefully get us
   closer to a unified LLVM and GCC bootstrap.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Stdenv
Development

Successfully merging this pull request may close these issues.

5 participants