[build] Remove intermediate pipeline so that we can guarantee that we always have a fresh swift-dispatch when running swift tests. #65829
Conversation
|
@swift-ci test |
|
@swift-ci test linux platform |
|
@al45tair, would you run the linux CI on this again? The month-old results have been removed. |
|
@swift-ci test linux platform |
|
@swift-ci test Windows platform |
|
Linux CI shows libdispatch-dependent tests running again, including several Concurrency tests that have long not been run because of this libdispatch dependency on the linux CI, with several now failing because of some configuration issue: They all appear to fail because they cannot find the Swift module for Dispatch for some reason: I don't see this error when running the compiler validation suite on Android without this pull, ie most of these tests pass for me, so this appears to be something specific to the linux CI. |
Spoke too soon, as I see this on Android too now, will investigate. |
|
OK, found the problem: libdispatch requires that non-Darwin platforms have its generic modulemap installed when built for Swift, but wherever that was done was probably removed at some point this year. I have been running these trunk compiler validation tests once a month natively on Android, and I noticed a couple weeks ago that some extra modulemap files were in After that, I hit these test failures natively on Android too with the Aug. 10 trunk snapshot, since the validation suite looks in the libdispatch source directory directly for that module. I confirmed that was the problem by manually copying Looking into the source now, @compnerd, looks like these tests all broke with your swiftlang/swift-corelibs-libdispatch#785, but nobody knew because these tests were not being run on the linux CI. Any idea how we can fix this now? |
|
I took a look at Saleem's libdispatch pull and tried something similar natively on Android, gottesmm#2, which fixed the libdispatch test failures for me and should do the same on this linux CI. @gottesmm, if you would just get my pull into this branch, we can make sure it works on the linux CI. |
|
@yln, not getting a response on this, maybe you can get this looked at. |
If you haven't already, I'd try pinging him in Slack. I'm definitely not the best person to ask if he isn't responding (because I'm in a different country, whereas others work in the same building as him and could drop by his office). You could try @tbkka if you can't get a reply from @gottesmm? |
… 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
gottesmm
force-pushed
the
rdar89046735
branch
from
a87e50d to
84a32c0
Compare
|
@swift-ci test |
|
@finagolfin sorry, I didn't see your ping. I pulled in your commit into this PR and restarted the testing. |
|
|
@swift-ci test linux platform |
|
@finagolfin Looks like there is still 1 Linux failure. I am unsure if it is related. |
|
OK, that test fails for me on Android too, looks like it is just written wrong, as it checks for different output than it is producing. I'll look into a fix after an hour or two, as I've got some other work to do now, feel free to try something else in the meantime. |
|
Got a moment to look into it: the test expects the @etcwilde, you last modified that line of this single failing test, any idea what's going on with this string output issue? If we can get it fixed now, we can get this test running on the CI again. |
|
@gottesmm, disabled that last test on linux in my pull, one last CI run and we can get this in. Would be good to get this in before the branch, so all these tests will be run for 5.10 too. |
|
@finagolfin I pinged @etcwilde |
|
It looks like the formatting of the task priority CustomStringConvertible changed in this dispatch. It seems like the output should be extension TaskPriority: CustomStringConvertible {
public var description: String {
"TaskPriority(rawValue: \(rawValue))"
}
}But according to the output, printing doing something else. It looks like the priorities are matching correctly as expected. I think the fix is to just define a function or just extract the raw task priority value and use that instead of relying on the e.g. remove the // ...
static func main async {
print("main priority: \(Task.currentPriority.rawValue)") // CHECK: main priority: [[#MAIN_PRIORITY]]
await test_detach()
// ...
}
let a1 = Task.currentPriority
print("a1: \(a1.rawValue)") // CHECK: a1: [[#MAIN_PRIORITY]]
// ...We're just checking that we return back to a lower priority correctly. |
|
@etcwilde, will do, this test works on macOS and the simulators now so will need to rerun on the CI to make sure that change won't break. Is this a platform-breaking bug that needs to be looked into, since invoking the |
|
Hmm, give me a sec. #63543 added a This should be conflicting with the conformance in the test, so I would expect all of the platforms to have issues here (maybe an error for a redeclaration or ambiguous overload or something), but certainly not choosing a different definition. It might be worth getting a type-checker person in here. There should definitely be consistent behavior across platforms though, assuming they're both building for Swift "5.9" or newer. |
|
@swift-ci please test |
|
(Trying to pull logs off of all platforms so I can see if there's anything obviously different between the two) |
|
From a quick conversation, having multiple conformances to the same protocol might just be UB. Unfortunate that it's not a reliable error message, but I haven't thought enough about what one would need to do to reliably detect it. If that's the case, we delete the protocol conformance above and we can either go with the official 5.9 description definition, or we can pull the rawValue and match against that. I think the regex for the raw value will maybe be a bit easier, but the string description might be more reliable in case someone decides they want different raw values for the priorities. |
|
Okay, it is most certainly UB. We should remove the duplicate conformance from the test and fix the thing the test is checking for. I'll let you choose what you want to match against. |
|
@etcwilde, thanks for the explanation, I've updated my commit to match those new string descriptions. @gottesmm, if you would merge my pull into this one, it should pass the linux CI now- I ran it on Android to make sure it works- though the macOS CI should also be rerun to make sure the test changes don't break there. |
|
@finagolfin sounds good |
…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.
gottesmm
force-pushed
the
rdar89046735
branch
from
84a32c0 to
de41f83
Compare
|
@swift-ci test |
|
Passed all CI now but looking at the log, I notice that all the Swift corelibs up to llbuild are now built before the compiler validation suite is run. This could cause problems when someone is trying out a compiler pull and something inscrutable breaks when building swift-corelibs-foundation, rather than the much cleaner tests in the compiler test suite. @gottesmm, what do you think about replacing your commit with this patch instead, simply moving libdispatch into the earlier build-script pipeline? This will make sure libdispatch is built before the compiler validation suite is run, thus enabling all these libdispatch and concurrency tests again, but keeps all the other corelibs split off in a separate pipeline for later. Let me know what you think. |
|
Pinging @gottesmm again, let me know what you think of the libdispatch change I suggested above over the weekend. |
|
@finagolfin I would rather merge this since it works and then do that as a separate patch. What do you think? |
|
@gottesmm, fine by me, I will submit that once this is in. |
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
Fixes #53973