-
Notifications
You must be signed in to change notification settings - Fork 166
Revert "Create new target for common code (#1331)" #1352
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
This reverts commit 70a8a0d.
|
@swift-ci test |
d-ronnqvist
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.
It was my understanding that issues with the Windows CMake build would not be blocking anything. If that's not correct, can we revert #818 instead?
|
@d-ronnqvist Unfortunately it actually blocks merging PRs on swiftlang/swift because we use the CMake build setup for Windows CI and it's a mandatory check. Given that it's affecting widely, it might be worth reverting this to get the CI back to green as soon as possible 🙏 Also reverting #818 alone unlikely solve the issue because we already use the CMake setup in the CI. |
|
That's very different from how I was led to believe that the DocC Windows CMake configuration would work at this early stage. Because we have no CI for that ourselves and insufficient instructions on how to locally update and locally verify changes, I would suggest that we revert #818 and any CI configuration that relies on it. |
|
I agree that we should have a CI check in swift-docc for CMake build health before relying on the CMake build in the main CI pipeline. @compnerd What do you think about reverting swiftlang/swift#71422 until we have a proper CI setup in swift-docc? |
|
@kateinoigakukun, probably pretty late where Saleem is in US: what do you think about getting #1351 merged for now to unblock the Windows CI, since it is the smallest change, and then Saleem can chime in on a better change later? |
|
@finagolfin Unfortunately, Doug's patch alone doesn't fix the build 😢 (see https://ci-external.swift.org/job/swift-PR-windows/47991/console) |
|
Any one of the three that fixes it for now is fine with me, your choice. 👍 |
|
IMHO, merging this PR is the quickest way to get the CI green again, since we already know it works. But I understand @d-ronnqvist's concern, so I just prepared a PR reverting build.ps1 change to stop relying on CMake for DocC build: swiftlang/swift#85534 I'll wait CI for swiftlang/swift#85534 and merge it ASAP, unless this PR gets merged. |
|
I believe this was obsoleted by #1356. If that's incorrect, feel free to reopen this PR. |
This reverts #1331 to repair CMake build on Windows CI
https://ci-external.swift.org/job/swift-PR-windows/47966/console