-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Build and install swift-testing in toolchains #75515
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
Conversation
243d971 to
b8740cf
Compare
b8740cf to
f11ef1e
Compare
edymtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with regard to the build-script changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating my feedback from the original PR!
|
swiftlang/swift-testing#581 |
04e364a to
74a36f8
Compare
|
@swift-ci Please smoke test |
74a36f8 to
b70f07e
Compare
|
swiftlang/swift-testing#581 |
|
swiftlang/swift-testing#581 |
|
swiftlang/swift-testing#581 |
stmontgomery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you so much for picking up this effort!
compnerd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The windows build support changes look good modulo minor tweak.
utils/build.ps1
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this next to the call to Build-SwiftTesting above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't move this right next to Build-SwiftTesting call because it is for $WindowsSDKArchs not $HostArch. To what line you want to move this exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why is it being built for each architecture for the SDK? The macros run at compile time, so we should only have to build it for the host that the toolchain is being built for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, TestingMacros is built for the host only, Testing is built for each SDK arch. That's how I implemented. Am I missing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macros should be running with the compiler (host) not the SDK environment (target). So, I'm not sure I understand why we are building it in all these cases.
|
swiftlang/swift-testing#581 |
* Remove SwiftSyntax product dependency * Correct CMAKE_BUILD_TYPE
* Basically following XCTest scheme. * Build TestingMacro separately from Testing library and install it to the toolchain's `bin` * Testing swift-testing itself is TODO
d17634f to
9aa6f06
Compare
45c1be3 to
bd5b4b0
Compare
|
swiftlang/swift-testing#581 |
bd5b4b0 to
9460969
Compare
|
swiftlang/swift-testing#581 |
|
swiftlang/swift-syntax#2793 |
|
swiftlang/swift-syntax#2793 |
|
swiftlang/swift-syntax#2793 |
|
swiftlang/swift-syntax#2795 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Manually iterate tor the cross compile hosts. | |
| # Manually iterate the cross compile hosts. |
and other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) I would suggest to put the classes for SwiftTesting macros in a separate file, to maintain the convention that one file targets one build product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) Impl in this and SwiftTestingImpl can suggest this implements a build-script-impl product -- but I cannot think of a better alternative
| class SwiftTestingMacrosImpl(cmake_product.CMakeProduct): | |
| class SwiftTestingMacrosCMakeShim(cmake_product.CMakeProduct): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) In this and other places, _impl can suggest this is part of a build-script-impl product
| def _install_impl(self, host_target): | |
| def _install_with_cmake(self, host_target): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) It may make sense to state the intent of the version parameter so not to "overload" it with additional meanings in the future
| def generate_darwin_toolchain_file(self, platform, arch, version=None): | |
| def generate_darwin_toolchain_file(self, platform, arch, macos_deployment_version=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) override_deployment_version may imply this applies to Linux
| self, host_target, override_deployment_version=None): | |
| self, host_target, override_macos_deployment_version=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to override this only if the macOS deployment version is lower than the desired one -- this way if the default changes or the user uses a newer one we are not stuck with an old version
This suggestion is an expedient way to achieve this in this context -- we should likely prefer the functions defined in https://github.com/swiftlang/swift/blob/main/utils/build_swift/build_swift/versions.py
| override_version = '10.15' | |
| def version_parse(version_string): | |
| return list(map(lambda x: int(x), version_string.split("."))) | |
| if version_parse(self.args.darwin_deployment_version_osx) < (10, 15): | |
| override_version = '10.15' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a stab using the Version class:
| override_version = '10.15' | |
| from build_swift.build_swift.versions import Version | |
| if Version(self.args.darwin_deployment_version_osx) < Version('10.15'): | |
| override_version = '10.15' |
9460969 to
2e6cc34
Compare
Also, build them for all hosts including cross compiling host.
2e6cc34 to
e98c5ea
Compare
(Supersedes #74582)
swift-testingtoupdate-checkout'smainschemeSwiftTestingproduct toswift_build_supportand integrate it to the build pipelineBuild-SwiftTestingMacrosandBuild-SwiftTestingfunctions, and integrate it to the pipelineswift-testingis not implemented in this PR.Build presets:
mixin_linux_installationmixin_osx_package_basebuildbot_all_platforms,tools=RA,stdlib=RAbuildbot_incremental,tools=RA,stdlib=RAbuildbot_incremental_linuxInstall locations:
Testing.frameworkin the Xcode SDK):