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.

Sign up for GitHub

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

Jump to bottom

Support iOS 13 in cross #93596

Merged
merged 7 commits into from
Sep 11, 2020
Merged

Support iOS 13 in cross #93596

merged 7 commits into from
Sep 11, 2020

Conversation

matthewbauer
Copy link
Member

Motivation for this change
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.

@matthewbauer matthewbauer changed the base branch from master to staging July 21, 2020 19:32
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 1-10 labels Jul 21, 2020
@matthewbauer matthewbauer force-pushed the ios-13 branch 2 times, most recently from 511f5d3 to 7f4d3f4 Compare July 22, 2020 16:56
Ericson2314
Ericson2314 reviewed Jul 22, 2020
View reviewed changes
pkgs/os-specific/darwin/xcode/sdk-pkgs.nix Outdated
'' + stdenv.lib.optionalString (sdk.platform == "iPhoneOS") ''
echo "-miphoneos-version-min=${minSdkVersion}" >> $out/nix-support/cc-cflags
echo "-Wl,-platform_version,ios,${targetPlatform.minSdkVersion or "9.0"},${sdk.version}" >> $out/nix-support/cc-cflags
'';
Copy link
Member

@Ericson2314 Ericson2314 Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should move these to the binutils override above

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - that avoids the warning

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 think the c compiler should know about minSdkVersion, so I readded miphoneos-version-min.

Copy link
Member

Choose a reason for hiding this comment

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

Weird it knows about it, but can't also send it to the linker, ugh.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sends the min sdk version to the linker, but not the sdk version. It looks like this might fix that:

https://reviews.llvm.org/D71579

@matthewbauer matthewbauer force-pushed the ios-13 branch 2 times, most recently from dac26f5 to 3c830bc Compare July 23, 2020 03:15
@matthewbauer
xcode/sdk-pkgs.nix: set -platform_version in addition to -miphoneos-v…
95eabdf 
…ersion-min

The App Store looks at LC_VERSION_MIN_IPHONEOS to verify you have a
new enough SDK version. This is not just the minimum version, but also
the sdk version used. When the linker can’t figure it out, it tries to
infer it from the sdk path[1]. When no sdk version is found, it
defaults to just using the -miphoneos-version-min value[2]. So, to make
sure we don’t rely on inference (which doesn’t work in the current
directory structure), we have to specify -platform_version.

[1]:
https://github.com/tpoechtrager/cctools-port/blob/43f32a4c61b5ba7fde011e816136c550b1b3146f/cctools/ld64/src/ld/Options.cpp#L5355-L5376
[2]: https://github.com/tpoechtrager/cctools-port/blob/43f32a4c61b5ba7fde011e816136c550b1b3146f/cctools/ld64/src/ld/ld.hpp#L58
@matthewbauer matthewbauer mentioned this pull request Jul 30, 2020
Merged
13 tasks
@matthewbauer matthewbauer merged commit 9112a9d into NixOS:staging Sep 11, 2020
@thefloweringash
Copy link
Member

thefloweringash commented Sep 19, 2020 •
edited
Loading

The cctools bump seems to have added a warning on linking:

$ nix-shell ~/src/nixpkgs-staging -A hello --run 'clang -v -x c -o /dev/null /dev/null'  2>&1 | grep warning
ld: warning: passed two min versions (10.12.0, 10.12) for platform macOS. Using 10.12.

This warning seems harmless enough, but ruby's configure script treats this as a problem with LDFLAGS.

$ nix-build -A ruby
[...]
checking whether LDFLAGS is valid... no
configure: error: something wrong with LDFLAGS=""
builder for '/nix/store/2b4wc2navpjw3hm4aijyhsqrfc3yzav9-ruby-2.6.6.drv' failed with exit code 1

As pointed out in the thread above, clang adds version args when calling the linker, which duplicates the flags set in ${darwin.binutils}/nix-support/libc-ldflags-before and causes the warning.

Just as a test, reverting 3c6bd61 is sufficient to build ruby.

@zowoq zowoq mentioned this pull request Oct 24, 2020
Closed
@thefloweringash thefloweringash mentioned this pull request Feb 5, 2021
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 left review comments

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewbauer @thefloweringash @Ericson2314