Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disable CMake bootstrap by default #7089

Merged
merged 1 commit into from Nov 13, 2023
Merged

Disable CMake bootstrap by default #7089

merged 1 commit into from Nov 13, 2023

Conversation

neonichu
Copy link
Member

It sounds as if we shouldn't need this anymore.

@neonichu
Disable CMake bootstrap by default
303cbc7 
It sounds as if we shouldn't need this anymore.
@neonichu neonichu self-assigned this Nov 13, 2023
@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu neonichu merged commit 72b184f into main Nov 13, 2023
5 checks passed
@neonichu neonichu deleted the disable-cmake-bootstrap branch November 13, 2023 21:15
@finagolfin
Copy link
Contributor

finagolfin commented Nov 14, 2023 •
edited

What is the intent of this pull? I added this bootstrap flag in #3000 three years ago, but it does nothing for the native CI when enabled. I checked the CI runs for this pull and it had no effect on linux or macOS (the Windows CI doesn't build SwiftPM or use the Python build scripts). There are three common scenarios when building SwiftPM:

  1. Building a full native toolchain from source - this flag does nothing in that case, as shown on the CI.
  2. Building a native SwiftPM alone with a prebuilt native Swift toolchain that has a prebuilt SwiftPM - I showed how to do that with build-script in my original pull, and this change will get that working without the kind of additional patching shown there.
  3. Cross-compiling a Swift toolchain by using a native Swift toolchain that has a native SwiftPM - this is the usecase I added this for, and I can remove the invocation of this flag that I added for non-Darwin cross-compilation now that it is enabled by default here.

If you want to enable 1. also, you will need some way for the bootstrap script to distinguish between the freshly-built trunk toolchain in /home/build-user/build/buildbot_incremental/toolchain-linux-x86_64/, which doesn't have SwiftPM, and the prebuilt 5.8.1 toolchain on the linux CI at /opt/swift/5.8.1/, which does. Currently, the bootstrap script isn't aware of and doesn't use the prebuilt 5.8.1 toolchain for the SwiftPM build at all, so simply changing this default does nothing on the CI.

@neonichu
Copy link
Member Author

Thanks, good catch. I had only verified that this does what I expected locally but not on CI. So seems like this isn't a sufficient change.

@neonichu
Copy link
Member Author

To answer your question about the intent, we'd like to remove the CMake portion of the build entirely at some point, I (mistakenly) thought this would be an easy way to preflight that idea in all workflows that use bootstrap.

neonichu added a commit that referenced this pull request Nov 14, 2023
@neonichu
Make skip-cmake-bootstrap unconditional
95645f3 
Follow up to #7089

I don't necessarily expect this to work as-is.
@neonichu neonichu mentioned this pull request Nov 14, 2023
Closed
neonichu added a commit that referenced this pull request Nov 14, 2023
@neonichu
Make skip-cmake-bootstrap unconditional
b6b3632 
Follow up to #7089

I don't necessarily expect this to work as-is.
@neonichu
Copy link
Member Author

Going to revert this since it is incomplete as-is.

neonichu added a commit that referenced this pull request Nov 17, 2023
@neonichu
Revert "Disable CMake bootstrap by default (#7089)"
5626acf 
This reverts commit 72b184f.
@neonichu neonichu mentioned this pull request Nov 17, 2023
Merged
neonichu added a commit that referenced this pull request Nov 28, 2023
@neonichu
Revert "Disable CMake bootstrap by default" (#7108)
9c22c6c 
Reverts #7089

This is incomplete and needs a significant amount of work to actually do
what we want.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@tomerd tomerd tomerd approved these changes

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@neonichu @finagolfin @MaxDesiatov @tomerd