Skip to content

[5.9][build] Move libdispatch earlier in the build so it can be used by the compiler validation suite #69035

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

Closed
wants to merge 3 commits into from

Conversation

finagolfin
Copy link
Member

Cherrypick of #65829 and #68565

Explanation: The libdispatch tests and several concurrency tests in the validation suite were inadvertently disabled a couple years ago. This enables them again by making sure the target libdispatch is built first and using the new dispatch-vfs-overlay.yaml it now generates.

Scope: Enabling compiler tests again

Issue: #53973

Risk: negative, as it is only testing more code, thus lowering risk

Testing: Passes all CI on trunk for the last three weeks, and I've been doing this manually on Android for years without a problem.

Reviewer: @gottesmm

gottesmm and others added 3 commits October 7, 2023 15:12
… always have a fresh swift-dispatch when running swift tests.

In the fullness of time, we want to split the full build-script-impl pipeline so
that we can begin moving library like products (libdispatch, foundation) from
build-script-impl into build-script. We are not there yet since some of swift's
concurrency tests have a dependency on swift dispatch being built. This breaks
the build and we need to extract those tests into a separate product. But for
now, this makes sense to repair the build.

rdar://89046735
…g/swift-corelibs-libdispatch#785

This allows the tests that use libdispatch to find its modulemap, plus add the
libdispatch compilation flags to one test that was missing them and fix one
async test on linux.
…uilt before the compiler tests are run

This reverts 0132841 but moves libdispatch into the earlier pipeline, so that
it alone is built before the compiler validation suite is run.
@finagolfin finagolfin requested a review from a team as a code owner October 7, 2023 10:00
@finagolfin
Copy link
Member Author

@Azoy, please run the CI on this.

@finagolfin
Copy link
Member Author

@al45tair, mind reviewing?

@finagolfin
Copy link
Member Author

@bnbarham, please run the CI.

@al45tair
Copy link
Contributor

@swift-ci Please test

@al45tair
Copy link
Contributor

@al45tair, mind reviewing?

I'm not a branch manager, so I don't think my reviewing it would help you much. @tbkka might be able to take a look?

@finagolfin
Copy link
Member Author

I checked the linux build log here and for a recent 5.9 build log without this pull and confirmed that several libdispatch and concurrency tests are re-enabled on this 5.9 branch and passing with this pull.

@finagolfin
Copy link
Member Author

@artemcm, mind reviewing?

@finagolfin
Copy link
Member Author

@compnerd, would you review?

@finagolfin
Copy link
Member Author

@DougGregor, this simply re-enables several concurrency tests for the 5.9 release branch on linux that were inadvertently disabled a couple years ago, please review.

@finagolfin
Copy link
Member Author

@bnbarham, can we go ahead and get this in? Adding tests alone only lowers risk by checking for more bugs, changing no behavior of the release toolchain.

@airspeedswift
Copy link
Member

I am not really seeing why it’s important to merge this to 5.9. It’s already on 5.10 and main, that seems like enough.

@finagolfin
Copy link
Member Author

@airspeedswift, it's not important. Since it changes no behavior in the toolchain, I think it's a good way to lower the risk of regressions creeping into this 5.9 release branch from other commits, by running more tests on them.

If you don't want to do that, feel free to close.

@finagolfin
Copy link
Member Author

Ping @shahmishal, would be good to get a decision on this.

@airspeedswift
Copy link
Member

Even though this is just tests, I'd rather keep the 5.9 branches as minimal as possible – just critical bug fixes at this point. Let's get this fixed on 5.10+

@finagolfin
Copy link
Member Author

Let's get this fixed on 5.10+

Already merged and working well there, no problem, which is why I suggested it here too, but not a big deal if you'd rather not.

@finagolfin finagolfin deleted the release/5.9 branch November 29, 2023 16:13
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.

4 participants