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

Jump to bottom

swift: 4.3.2 -> 5.0.1 #60744

Merged
merged 4 commits into from May 7, 2019
Merged

swift: 4.3.2 -> 5.0.1 #60744

merged 4 commits into from May 7, 2019

Conversation

mroi
Copy link
Contributor

@mroi mroi commented May 2, 2019

I updated Swift to version 5.0.1. This needed relatively minor changes, the biggest one is the addition of the NIX_LDFLAGS_BEFORE variable to help the internal clang that is being built during the Swift build process.

I also updated the patches and added two additional ones. One has been filed as an upstream build issue, so it can be removed once this trickles down. The other causes the Swift build system not to create its own versions of libicu and libc++.

Some older build tweaks could be removed, because they no longer appear to have an effect.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@mroi
swift: 4.2.3 -> 5.0.1
daca045 
update Swift
* new tweaks to the build process are mostly documented inline
* the swiftc wrapper needs the linker path for libicu
* some older tweaks could be removed
* remove fuzz from patches by updating line numbers
* one build fix to libdispatch has been filed upstream
@mroi
swift: enable tests (WIP)
4ba3923 
add check phase for running some of the Swift test suite

A small number of tests is failing, which needs investigation,
so the check phase remains disabled by default.
@ofborg ofborg bot requested a review from dtzWill May 2, 2019 10:35
@dtzWill
Copy link
Member

dtzWill commented May 3, 2019

Awesome, thank you!!

This looks great! I can build it and seems to work as expected!

I think this can go in as-is-- I'll see about a few details and will make a follow-up PR if they turn out useful/desirable :).

@dtzWill
Copy link
Member

dtzWill commented May 3, 2019

Oh, I just saw the tests bit. Should this wait for that to be sorted, then?
Wow, tests! That'd be great! :)

dtzWill
dtzWill reviewed May 3, 2019
View reviewed changes
pkgs/development/compilers/swift/default.nix
@@ -120,6 +120,9 @@ let
builder = ''
# gcc-6.4.0/include/c++/6.4.0/cstdlib:75:15: fatal error: 'stdlib.h' file not found
NIX_CFLAGS_COMPILE="$( echo ${clang.default_cxx_stdlib_compile} ) $NIX_CFLAGS_COMPILE"
Copy link
Member

Choose a reason for hiding this comment

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

Whhhyyyyyyy

  1. Do we really not build with clang? 😢
  2. WTF putting CXXFLAGS in CFLAGS, yikes
  3. WTF the mix of headers, compilers, runtime libraries-- do we even know what combination is used? Is it consistent throughout the various builds?!

..... And this isn't even something you're adding, so don't worry about it. But... :(. D'oh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is pretty gnarly. The way I see it, we do indeed use clang, but clang has gcc as a dependency and uses some of its compiler-shipping headers and startup files. The need for these extra flags however is a bit weird for me as well. I tried twice (but just briefly) to get rid of it, because I get bad vibes from it too, but so far failed to diagnose the actual problem. Nix’s clang should be able to find its stuff without help, but somehow it does not.

@dtzWill
Copy link
Member

dtzWill commented May 4, 2019

Well, here's a few tweaks-- what do you think? If you like them feel free to pick them, or just say so and I can do it or make a PR or w/e. Just wanted to share even if this PR isn't merged yet.

  • Build w/default clang instead of old clang5: dtzWill@6660290
  • Move most inputs expected to be on PATH to nativeBuildInputs: dtzWill@a365a5a -- least confident about this, but seems to work. And honestly this package is very likely to not work either way for cross, so it's mostly on principle I guess.
  • Editor integration: dtzWill@f9332ff -- installs at least vim and emacs bits from upstream. May need some post-processing / plumbing to actually be used, I can at least tackle that for vim at some point. Even so, better to have them available for use (even if they need some minor moving around), since building takes so long xD.

@mroi
Copy link
Contributor Author

mroi commented May 5, 2019

I would not wait for the tests to work out. These need some extra work and do not appear to be critical. The failures revolve around build problems that appear to be Nix-specific. They did not immediately indicate to me that the resulting swift compiler was broken.

Regarding your additional commits:

  • I also tested building with default clang and would agree to use the stable version. Hardcoding clang_5 does not appear to have an obvious benefit.
  • Regarding nativeBuildInputs: I think it conveys the intent of these dependencies better, so even if cross-builds are currently untested, I think this is a good idea to separate the dependencies we are confident about.
  • Installing the editor integration sounds like a good idea as well. I wonder why the build presets only include it in the Darwin builds. Maybe this change should be propagated upstream?

In summary: I would vote for including all your proposed changes. If you want, I can include those in my pull request and retry the entire build at my end tomorrow.

@mroi
Copy link
Contributor Author

mroi commented May 6, 2019

The way I see this, the original Nixpkgs repo already builds swift with default clang instead of hardcoding clang_5. So there must be something else going on.

The other two changes check out fine on my end. If you want, I can easily add them to this pull request.

@dtzWill
Copy link
Member

dtzWill commented May 6, 2019 via email

@mroi
Copy link
Contributor Author

mroi commented May 6, 2019

I just pushed the additional commits.

@ofborg ofborg bot requested a review from dtzWill May 6, 2019 17:11
@dtzWill
Copy link
Member

dtzWill commented May 7, 2019

Thank you for all your work on this!! LGTM!

@dtzWill dtzWill merged commit 9ebc6ad into NixOS:master May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mroi @dtzWill