-
Notifications
You must be signed in to change notification settings - Fork 30
Simplify build script #2733
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
Simplify build script #2733
Conversation
610d50f
to
81af4f3
Compare
@@ -909,36 +909,6 @@ function(swift_icu_variables_set sdk arch result) | |||
endif() | |||
endfunction() | |||
|
|||
if(SWIFT_PRIMARY_VARIANT_SDK STREQUAL "WASI") |
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.
I am unsure about moving ICU download and unpacking process from CMake to a shell script. If, for example, we're interested in making SwiftWasm work on Windows, wouldn't we need to move it back to CMake again?
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 main purpose is minimizing platform specific code from cmake build system.
This kind of external library information should be separated from the use-site because the use-site doesn't care which flavor of libraries they are using.
If we want to support non Unix-like platform as a host environment, we need use rewrite a downloader script in more portable language. But this separation allows the CMake build system to be more flexible.
The build system flexibility will be helpful when we stop forking and only build the toolchain like compnerd/swift-build
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.
Isn't CMake more portable than bash from this perspective then? And would moving this code a separate CMake file make this more maintainable?
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.
I mean the use and injection of build parameters should be separated.
If using and injecting are integrated in a same script, then the build system will only be able to build a single flavor.
I think such a change is unlikely to be accepted upstream.
Writing a script in CMake to download a file for injection is fine, but that script should be isolated from the project's build system.
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.
That makes sense, thanks for the clarification!
I'm thinking about playing with Swift on Windows at some point, and that made me consider the future of SwiftWasm on that platform as well. Proliferation of shell scripts in our build system will only complicate porting efforts, so I wonder if we should have a coherent strategy for how this evolves moving forward...
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.
I'll just mention I sometimes work in really bad network conditions, so having all dependencies downloaded before build is essential. I've always commented out this ICU download in my local copy.
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.
Seems legit 👍
On a somewhat unrelated note, I hope we can make these scripts truly cross-platform in the future. If a separate CMake invocation for downloading dependencies is unsuitable, maybe Python could work? Especially as it's already used in different places when building the toolchain.
Python is one of the most possible options, I think. 👍 |
* [WASM] Simplify Foundation build script * [WASM] Remove WASI ICU related code from CMake build system * Refactoring build-toolchain script * [WASM] Fix prebuilt libraries path * [WASM] Always install ubuntu wasi-sdk * Fix unarchive command
swift-source/build-sdk
instead of scattering in the source dir.This PR should be merged before swiftwasm/swift-corelibs-foundation#267