[1/2][build_script.py] Add "test" subcommand #57
Merged
Conversation
This file contains hidden or 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
modocache
added a commit
to modocache/swift-corelibs-xctest
that referenced
this pull request
Feb 28, 2016
This builds upon the subcommand system added in swiftlang#57, to add an install command. As that pull request describes, because there is no way to test or install XCTest without also building the project, the current Swift build script ends up building XCTest three times. This adds an "install" subcommand that allows an already built XCTest to be installed.
modocache
changed the title
This "FIXME" describes a problem with the swift-corelibs-xctest build script when invoked in the context of the greater Swift build script: https://github.com/apple/swift/blob/6ccf5da2dfc8a74c84b2fd99c32dcdf2255699d7/utils/build-script-impl#L2070-L2072 The problem is that there is currently no way to test an *already built* XCTest.so. Instead, the XCTest build script re-builds the entire project, then tests the newly build XCTest.so. Add a "test" subcommand to build_script.py, which allows users to test an already built XCTest.so: ``` $ ./build_script.py test --swiftc `which swiftc` /path/to/build/dir ``` Users may still choose to build *and* test, by using the old build_script.py invocation: ``` $ ./build_script.py --swiftc `which swiftc` --test ```
modocache
force-pushed
the
build-script-subcommands-test
branch
from
1f719f3 to
2cbc592
Compare
modocache
added a commit
to modocache/swift-corelibs-xctest
that referenced
this pull request
Feb 28, 2016
This builds upon the subcommand system added in swiftlang#57, to add an install command. As that pull request describes, because there is no way to test or install XCTest without also building the project, the current Swift build script ends up building XCTest three times. This adds an "install" subcommand that allows an already built XCTest to be installed.
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
This seems like a good thing to have. |
mike-ferris
pushed a commit
that referenced
this pull request
Feb 29, 2016
[1/2][build_script.py] Add "test" subcommand
modocache
added a commit
to modocache/swift-corelibs-xctest
that referenced
this pull request
Mar 1, 2016
This builds upon the subcommand system added in swiftlang#57, to add an install command. As that pull request describes, because there is no way to test or install XCTest without also building the project, the current Swift build script ends up building XCTest three times. This adds an "install" subcommand that allows an already built XCTest to be installed.
1 task
modocache
added a commit
to modocache/swift
that referenced
this pull request
Mar 2, 2016
Prior to swiftlang/swift-corelibs-xctest#57 and swiftlang/swift-corelibs-xctest#58, the swift-corelibs-xctest build script did not provide a way an already built XCTest.so to be tested or installed. As a result, the Swift build script would build XCTest several times. For example, the following invocation would build XCTest three separate times: ``` $ utils/build-script --xctest --test -- --install-xctest ``` Modifications to the XCTest build script now allow a prebuilt XCTest to be tested (via the `build_script.py test` subcommmand) and installed (via the `build_script.py install` subcommand). Use these in the Swift build script to build XCTest only once, then test and install the built version. Also, explicitly disable XCTest installation on OS X. XCTest's `build_script.py` only supports non-Darwin platforms, so running `utils/build-script --xctest -- --install-xctest` on OS X used to cause the build script to fail because of how that script invokes `swiftc`. It now fails with an explicit error indicating `--install-xctest` is unsupported.
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.
What's in this pull request?
This "FIXME" describes a problem with the swift-corelibs-xctest build script when invoked in the context of the greater Swift build script: https://github.com/apple/swift/blob/6ccf5da2dfc8a74c84b2fd99c32dcdf2255699d7/utils/build-script-impl#L2070-L2072
The problem is that there is currently no way to test an already built XCTest.so. Instead, the XCTest build script re-builds the entire project, then tests the newly build XCTest.so.
Add a "test" subcommand to build_script.py, which allows users to test an already built XCTest.so:
Users may still choose to build and test, by using the old build_script.py invocation:
Why merge this pull request?
The Swift build script is sort of disingenuous--maybe even dangerous--at the moment. When a user runs
swift/utils/build-script --xctest --test, I think they'd expect that XCTest is built, then tested. In reality, it is built, then built again, then the second version is tested.This gets even worse when we're talking about installing XCTest. In that case, it is:
This pull request is a large change just to prevent step (2) above, but it puts in place the subcommand mechanism that we'll need to fix the install step (4).
What downsides are there to merging this pull request?
Still, I think this is a significant improvement!