Skip to content

Conversation

@d-ronnqvist
Copy link
Contributor

This reverts commit 7b74f5c.

This is failing builds and causing noise. It was my understanding that this wouldn't be blocking any work. If that's not correct I feel that it's premature to have it.

@d-ronnqvist
Copy link
Contributor Author

I started a cross-repo test with this change here swiftlang/swift#84510 (comment)

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@compnerd
Copy link
Member

Reverting this is not really the right thing. CI is not something that can be setup outside of Apple - I cannot enable the GHA CI (and my PR has been pending review for a while now).

@etcwilde
Copy link
Member

The CMake build was broken in #1331 because folders were moved around without moving the associated CMakeLists files. Pulling the entire build that Windows uses because you broke it seems pretty heavy-handed.

@d-ronnqvist
Copy link
Contributor Author

The CMake build was broken in #1331 because folders were moved around without moving the associated CMakeLists files. Pulling the entire build that Windows uses because you broke it seems pretty heavy-handed.

The expectation for the approval of #818 was that the CMake build wouldn't be blocking or breaking anything.

I requested a CI setup as blocking feedback on #818 but allowed it to be approved/merged without it on the expectation that we wouldn't be responsible for it in the interim and that it wouldn't cause any breakages. Because those expectations turned out to not be true, I consider the approval of #818 to be invalidated.

@d-ronnqvist
Copy link
Contributor Author

In my opinion, if we keep these files in the repo then there can't be anything else that depends on them until after there is a CI setup that verifies that they're correct before a PR is merged. In other words; I expect the order of events to be:

  1. add the CMake files to the repo
  2. set up a CI that verifies the CMake files' correctness
  3. use the CMake files elsewhere

Right now it seems that 2 and 3 happened in the wrong order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants