[Commands] Fix swift-test to always build using the correct manifest.
#494
Conversation
|
I think this is an improvement in behavior (we currently sometimes just don't react to new test files, which is horrible), but the implementation is unfortunate. OTOH it does get rid of the horrible hack for finding the test binary path, so its not without benefits. The duplication between swift-build and swift-test is something we should eventually fix, but right now it is no worse than what we already have with swift-build and swift-package, and I still mostly want to focus on the lower level layers, I just happened to notice this was easy to fix. We have no good test coverage of |
|
Also, I started a CHANGELOG for us. 🎊 |
|
LGTM except the duplication :/ , maybe we can later refactor it to SwiftTool protocol. yay @ changelog |
| return possibleTestPath | ||
| } | ||
| // FIXME: We need to support testing in other build configurations, but need | ||
| // to solve the testability problem first. |
There was a problem hiding this comment.
what is the testability problem? @testable imports?
|
I think this PR is correct, but I'm holding off on merging it until it has some more test coverage. I also think I am going to preflight the change on swift-build-dev since it changes user interaction with |
|
Re: code duplication, I was thinking the same thing as @aciidb0mb3r, this might be a case in which SwiftTool comes in handy. |
|
I do also plan to provide a proper Command Line Option infrastructure at some point, in which case all the semantics and other data (help strings etc) surrounding an option could be implemented in a library and referenced from multiple places. |
|
Re: changelog: it might be a good idea to separate out the change notes that apply to the PackageDescription API vs those that are about the user-level semantics, such as behavior of the command line tool and command line tool options. |
|
I've actually been doing refactoring on trunk to help bring down some of the code duplication -- Unfortunately the remainder of the duplication is likely blocked on, as you say, having a good command line option infrastructure which supports composition of option sets (a somewhat non-trivial proposition). |
|
W.R.T. the CHANGELOG, I think the benefit of being able to read it top-to-bottom is the most important thing for the engineering notes. Things like PackageDescription API changes should also usually go through another, more formal process of documenting (e.g., a proposal) which should help distinguish the two. |
ddunbar
force-pushed
the
SR-1135
branch
3 times, most recently
from
55fceee to
7f03a5a
Compare
|
Rebased on top of all the refactoring that have landed on trunk since I first wrote this, it still involves some duplication of the options but otherwise the duplication is minimal. I am now happy enough with this to land it. |
|
👍 |
|
@swift-ci please test |
|
@swift-ci please test linux |
|
Ah, Linux CI failure is real, our own bootstrap script needs to be updated to pass |
ddunbar
force-pushed
the
SR-1135
branch
2 times, most recently
from
5868c3e to
058a192
Compare
- It will now always regenerate the build manifest (including cloning, etc, if necessary). This also means it now accepts the -Xcc (etc.) arguments that swift-build takes. Unfortunately that means they *must* be passed, if they were used before, but allowing an inconsistent build was a poor tradeoff for that feature. - The implementation is fairly gross, as we don't yet have the infrastructure we need to share all the common code between swift-build and swift-test (in particular to share the command line flags). - Resolves SR-1135.
|
@swift-ci please test |
|
4 participants
necessary). This also means it now accepts the -Xcc (etc.) arguments that
swift-build takes. Unfortunately that means they must be passed, if they
were used before, but allowing an inconsistent build was a poor tradeoff for
that feature.
we need to share all the common code between swift-build and swift-test (in
particular to share the command line flags).