swift: 5.0.2 -> 5.1.1 #72120
Merged
swift: 5.0.2 -> 5.1.1 #72120
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonringer
approved these changes
Oct 27, 2019
There was a problem hiding this comment.
nix-review passes on NixOS
executable seems to work (didn't really do much testing)
[16 built, 1 copied (0.0 MiB), 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/72120
1 package were build:
swift
it's a big boy
[nix-shell:/home/jon/.cache/nix-review/pr-72120]$ nix path-info -Sh ./results/swift
/nix/store/pyq15ga5rd4v6b9mv8nihi4bwnc5wha8-swift-5.1.1 1.3G
| fetch = { repo, sha256, fetchSubmodules ? false }: | ||
| fetchFromGitHub { | ||
| owner = "apple"; | ||
| inherit repo sha256 fetchSubmodules; | ||
| rev = tag; | ||
| rev = "refs/tags/swift-${version}-RELEASE"; |
There was a problem hiding this comment.
Suggested change
| rev = "refs/tags/swift-${version}-RELEASE"; | |
| rev = "swift-${version}-RELEASE"; |
There was a problem hiding this comment.
Thanks for catching this. I updated my branch with this change.
|
@GrahamcOfBorg build swift |
|
Thanks for testing on NixOS. I did some more testing of swift itself and did not find any problems. I agree it’s a fairly large closure, but the increment with this is update is just 12% (from 1283819840 to 1431600328). I think this is acceptable, given that more things are included and swift as a language grew quite a bit from version 4 to 5. |
|
I mean, it includes clang + llvm and quite a bit of a toolchain, not supprising given this.... just making an observation :) |
|
Understood. Yes, they require a modified, internal clang, which is a bit annoying. |
|
LGTM after basic building + test, will hopefully put through more serious paces in a few days but don't want to stall anything waiting on that. (worst case I file an issue/PR if anything problematic is encountered) |
|
I just ran the internal test suite by setting |
|
So close! Can we disable those, and do you have logs perhaps? Just curious what failures we run into. I think this is ready as-is, but even better if we can start testing any part of (or most of) this! |
|
Here are the log snippets of the failing tests: Driver/static-stdlib-linux.swiftstdlib/CUUID.swiftPython/build_swift.swiftI already tried some obvious fixes, but so far have not been successful in making these tests succeed. |
|
I have updated my changes and enabled the test suite. One failing test actually pointed to a problem I could fix. The other two look like build system issues to me and I disabled them for now. |
|
Apparently I have overlooked a build error. Please hold the line … |
|
I forgot to mention after my force push: |
jonringer
reviewed
Jan 14, 2020
There was a problem hiding this comment.
diff LGTM. However, the git history seems to have a lot of fixups. I don't expect this to just be a single commit, but it would be nice for a little consolidation.
https://nixos.org/nixpkgs/manual/#submitting-changes-making-patches
something like:
swift: refactor phase, patches, and docs
swift: 5.0.2 -> 5.1.1
swift: fix build and tests
* simplify versioning * uniform patch documentation * reorder phases so they read in order * reorder scripts for readability * GNU toolchain dir: handle sysroot for GNU toolchain like for C headers, so that GCC_INSTALL_PREFIX is actually used correctly
* fix UUID compilation error This was pointed out by Swift’s test suite. * enable tests after build Two broken tests disabled for now.
|
Thanks for your patience with this PR. |
|
all good, swift needs some love like every other package |
jonringer
approved these changes
Jan 16, 2020
|
Looks great, awesome work! Thanks! |
|
Thanks for merging! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
Update from 5.0.2 to 5.1.1. I did some cleanup in the preceding patches to minimize and clarify the changes. This PR therefore supersedes #68346.
Indexstore and Sourcekit are now mandatory in the build configuration we are using, so I added them, which was rather straightforward. I need to set
CCto the wrappedclangnow, so that building of swift packages picks up the wrapped compiler, helping the build to find correct headers and libraries.The patch for bug SR-10559 could be removed, because the fix is now upstream. A new workaround for LLVM bug 39743 got added (see
CCC_OVERRIDE_OPTIONS).Things done
sandboxinnix.confon non-NixOS)macOSTested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @dtzWill