Reactivate libdispatch tests on Linux #27940

yln · 2019-10-29T21:11:00Z

The swift-corelibs-libdispatch project had its build directory layout
changed [1] which silently deactivated the libdispatch tests on Linux,
because we use hard-coded paths to look for the required libdispatch
artifacts. This change adapts the paths to the new layout.

Thanks to Jordan Rose for noticing and diagnosing this [2].
SR-11568

[2] https://bugs.swift.org/browse/SR-11568
[1] swiftlang/swift-corelibs-libdispatch#515

The swift-corelibs-libdispatch project had its build directory layout changed [1] which silently deactivated the libdispatch tests on Linux, because we use hard-coded paths to look for the required libdispatch artifacts. This change adapts the paths to the new layout. Thanks to Jordan Rose for noticing and diagnosing this [2]. SR-11568 [2] https://bugs.swift.org/browse/SR-11568 [1] swiftlang/swift-corelibs-libdispatch#515

yln · 2019-10-29T21:11:14Z

@swift-ci Please test

yln · 2019-10-29T21:22:19Z

build/linux/libdispatch-linux-x86_64/src/libdispatch.so ->
build/linux/libdispatch-linux-x86_64/libdispatch.so

build/linux/libdispatch-linux-x86_64/src/libswiftDispatch.so ->
build/linux/libdispatch-linux-x86_64/libswiftDispatch.so

build/linux/libdispatch-linux-x86_64/src/swift/Dispatch.swiftmodule ->
build/linux/libdispatch-linux-x86_64/src/swift/swift/Dispatch.swiftmodule


➤ find build/linux/ -name libdispatch.so
build/linux/libdispatch-linux-x86_64/libdispatch.so
build/linux/swift-linux-x86_64/libdispatch-prefix/lib/libdispatch.so
build/linux/swift-linux-x86_64/libdispatch-prefix/src/libdispatch-build/libdispatch.so

➤ find build/linux/ -name libswiftDispatch.so
build/linux/libdispatch-linux-x86_64/libswiftDispatch.so

➤ find build/linux/ -name Dispatch.swiftmodule
build/linux/libdispatch-linux-x86_64/src/swift/swift/Dispatch.swiftmodule
build/linux/libdispatch_static-linux-x86_64/src/swift/swift/Dispatch.swiftmodule

yln · 2019-10-29T23:04:38Z

@swift-ci Please test

swift-ci · 2019-10-29T23:06:41Z

Build failed
Swift Test Linux Platform
Git Sha - fac3556

swift-ci · 2019-10-29T23:08:05Z

Build failed
Swift Test Linux Platform
Git Sha - fac3556

swift-ci · 2019-10-29T23:52:13Z

Build failed
Swift Test OS X Platform
Git Sha - fac3556

yln · 2019-10-30T00:16:10Z

lit now reports the libdispatch feature again.

lit.py: /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/lit.cfg:1678: note:
Running tests on Ubuntu-16.04
lit.py: /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/lit.cfg:1688: note:
Available features: <more text>, executable_test, fuzzer_runtime, **libdispatch**, lldb, <more text>

Among others the following test is now executed again on Linux.

PASS: Swift(linux-x86_64) :: stdlib/DispatchData.swift (7550 of 12291)

Please merge (despite other unrelated failures).

yln · 2019-10-30T00:18:15Z

@shahmishal It would be a nice feature of CI to get a warning in the form of a PR comment if the number of executed tests decreases for a given PR. This would help prevent "accidental disabling" of test.

gottesmm · 2019-10-30T00:20:09Z

Hey J! Is there any way we could make it so that if this happens again, we get an error instead of silently not running the tests?

…

On Oct 29, 2019, at 5:16 PM, Julian Lettner ***@***.***> wrote: lit now reports the libdispatch feature again. lit.py: /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/lit.cfg:1678: note: Running tests on Ubuntu-16.04 lit.py: /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/lit.cfg:1688: note: Available features: <more text>, executable_test, fuzzer_runtime, **libdispatch**, lldb, <more text> Among others the following test is now executed again on Linux. PASS: Swift(linux-x86_64) :: stdlib/DispatchData.swift (7550 of 12291) Please merge (despite other unrelated failures). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

yln · 2019-10-30T00:20:13Z

swiftlang/swift-corelibs-libdispatch#515
https://bugs.swift.org/browse/SR-11568

jrose-apple · 2019-10-30T00:22:58Z

Ah, no credit to me. I just tagged you when that bug was filed.

jrose-apple · 2019-10-30T00:23:21Z

test/lit.cfg

@@ -1020,16 +1020,16 @@ elif (run_os in ['linux-gnu', 'linux-gnueabihf', 'freebsd', 'windows-cygnus', 'w
    config.target_runtime = "native"
    config.target_swift_autolink_extract = inferSwiftBinary("swift-autolink-extract")

-    libdispatch_artifact_dir = make_path(config.libdispatch_build_path, 'src')
+    libdispatch_artifact_dir = config.libdispatch_build_path
+    libdispatch_swift_module_dir = make_path(libdispatch_artifact_dir, 'src', 'swift', 'swift')


This path seems…worse? Why the double nesting?

I have no idea. I just built under docker/Linux and looked where things ended up.
See my comment above: #27940 (comment)

I think it's worth fixing the path where things get built rather than adjusting the test logic to look there.

The "double" is because the swift overlay is in src/swift, and the swift modules/docs/interfaces are emitted into a swift directory.

yln · 2019-10-30T00:27:06Z

Hey J! Is there any way we could make it so that if this happens again, we get an error instead of silently not running the tests?

I can certainly come up with something, but I believe that this is general issue (not just libdispatch), best dealt with at the CI level.

gottesmm · 2019-10-30T00:31:25Z

What about if we add an environment variable that the CI sets that would cause us to error if we don't find it. (I.e. SWIFT_CI=1).

…

On Oct 29, 2019, at 5:27 PM, Julian Lettner ***@***.***> wrote: Hey J! Is there any way we could make it so that if this happens again, we get an error instead of silently not running the tests? I can certainly come up with something, but I believe that this is general issue (not just libdispatch), best dealt with at the CI level. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

finagolfin · 2019-10-30T07:38:28Z

I found and reported this issue, as I had periodically been running the tests locally since the beginning of the year and noticed these disabled tests when looking at prior test logs. I could have easily submitted this patch fix myself, but it wouldn't have been robust to any further moves in the libdispatch build directory. Since I got no response when I mentioned the regression on that libdispatch pull, I filed the bug so that the devs involved would actually discuss:

where the libdispatch build artifacts should go, and if that's final or might change again
whether lit.cfg should simply disable libdispatch testing alone if those libraries aren't found again, as done now and missed, or error and stop the whole test run

Without nailing those two details down, this is going to keep silently breaking, as mentioned by @gottesmm.

compnerd · 2019-10-30T15:10:12Z

I don't think that swift really needs to care about where the products go. Adjusting the path here to enable the tests first is a good approach. Subsequently, we should change the lit configuration to be substituted by CMake via lit.cfg.in to use the generator expressions to determine the location of the libdispatch content. We now expose them through export targets, and we can do something like $<TARGET_FILE_DIR:swiftDispatch> to get the location of the .dll/.so/.dylib and $<INTERFACE_INCLUDE_DIRECTORIES:swiftDispatch> to get the location of the swift module.

finagolfin · 2019-10-30T18:16:08Z

Sounds good, that should fix it.

jrose-apple · 2019-10-30T18:38:49Z

Swift may not care where the products go, but corelibs-libdispatch is still putting them in a silly place.

compnerd · 2019-10-30T19:26:27Z

I'm happy to change them in a follow up change to use a different path for the modules. Right now, we have multiple layers of things working around limitations of previous approaches, and keeping everything working is difficult enough. I'm pretty close to migrating Foundation, which should help remove some of this technical debt in the setup and making it easier to relocate the swift modules.

yln · 2019-10-30T20:55:59Z

@swift-ci Please test macOS

yln · 2019-10-30T21:28:13Z

What about if we add an environment variable that the CI sets that would cause us to error if we don't find it. (I.e. SWIFT_CI=1).

@gottesmm
I added this. We need to ensure this environment variable is added on our bots.

I don't think that swift really needs to care about where the products go. Adjusting the path here to enable the tests first is a good approach. Subsequently, we should change the lit configuration to be substituted by CMake via lit.cfg.in to use the generator expressions to determine the location of the libdispatch content. We now expose them through export targets, and we can do something like $<TARGET_FILE_DIR:swiftDispatch> to get the location of the .dll/.so/.dylib and $<INTERFACE_INCLUDE_DIRECTORIES:swiftDispatch> to get the location of the swift module.

@compnerd
I would need further guidance on how to do this.

yln · 2019-10-30T21:28:26Z

@swift-ci Please test

swift-ci · 2019-10-30T21:30:38Z

Build failed
Swift Test OS X Platform
Git Sha - 58002a7

swift-ci · 2019-10-30T21:30:48Z

Build failed
Swift Test Linux Platform
Git Sha - 58002a7

gottesmm · 2019-10-31T00:06:00Z

@yln instead of using an environment variable or the like, I think @shahmishal would prefer to have some sort of variable set in lit.

Can you also change the presets to set this variable as well?

yln · 2019-10-31T20:57:00Z

@swift-ci Please test

swift-ci · 2019-10-31T20:59:32Z

Build failed
Swift Test Linux Platform
Git Sha - a0c9a06

swift-ci · 2019-10-31T21:07:55Z

Build failed
Swift Test OS X Platform
Git Sha - a0c9a06

yln · 2019-11-01T00:12:08Z

@swift-ci Please test Linux

swift-ci · 2019-11-01T00:14:29Z

Build failed
Swift Test Linux Platform
Git Sha - 17c99a0128b245657458b171f80c1a12e6739cd5

yln · 2019-11-01T00:34:32Z

--lit-args=... specified on the command line or presets are not respected:
https://bugs.swift.org/browse/SR-11690

I removed my last 2 commits. I think wiring through CMake is the right solution. Once we have this maybe we can make building libdispatch mandatory on platforms that require it?

The swift_ci env var/lit param seems very brittle. It is also a specific solution for a general problem. I think this should be handled at the CI/Jenkins level outside of the messiness of build-script.

yln · 2019-11-11T20:03:10Z

ping

I would like to merge this so we start executing these tests on Linux again.

The two attempted, specific fixes have their own set of issues. My current understanding is that all --lit-args specified in presets are ignored.
https://bugs.swift.org/browse/SR-11690

My hope is that "wiring through CMake" as compnerd suggested will lower the chance of silently disabling the libdispatch tests.

I also want to re-iterate the my opinion that the "silent disabling of tests" is a general problem; it would be good to have a general solution:
#27940 (comment)

finagolfin

Confirmed that this pull enables the libdispatch and tsan tests again by building with the 11/08 snapshot on Android. I agree that the larger issues can be fixed later.

yln · 2019-11-12T16:18:07Z

Thanks @buttaface. And also thanks for bringing our attention to this in the first place.

finagolfin · 2021-10-31T16:44:48Z

@yln, I think this broke a couple months ago again, likely because #38888 split up the build pipeline so that cmark/llvm/swift are built and tested first.

The way it worked before was that all the build-script-impl products up through llbuild were built first before any tests were run, but with this new split, the compiler validation suite no longer finds a built libdispatch and the libdispatch and tsan tests from the compiler validation suite are not run on the linux CI.

yln · 2022-02-14T22:28:39Z

Fixed by @edymtt in #41135

yln self-assigned this Oct 29, 2019

Fix libdispatch Swift module directory

58002a7

yln requested review from jrose-apple, shahmishal, jakepetroules, ktopley-apple and compnerd October 30, 2019 00:19

jrose-apple reviewed Oct 30, 2019

View reviewed changes

yln requested a review from devincoughlin October 30, 2019 00:27

yln force-pushed the linux-libdispatch-tests branch from 17c99a0 to 58002a7 Compare November 1, 2019 00:11

finagolfin approved these changes Nov 12, 2019

View reviewed changes

yln merged commit 631305f into master Nov 12, 2019

yln deleted the linux-libdispatch-tests branch November 23, 2019 01:19

yln mentioned this pull request Oct 30, 2019

[SR-11568] libdispatch build reorganization inadvertently disabled tsan/libdispatch tests on linux CI #53973

Closed

Reactivate libdispatch tests on Linux #27940

Reactivate libdispatch tests on Linux #27940

Uh oh!

Conversation

yln commented Oct 29, 2019

Uh oh!

yln commented Oct 29, 2019

Uh oh!

yln commented Oct 29, 2019

Uh oh!

yln commented Oct 29, 2019

Uh oh!

swift-ci commented Oct 29, 2019

Uh oh!

swift-ci commented Oct 29, 2019

Uh oh!

swift-ci commented Oct 29, 2019

Uh oh!

yln commented Oct 30, 2019

Uh oh!

yln commented Oct 30, 2019

Uh oh!

gottesmm commented Oct 30, 2019 via email

Uh oh!

yln commented Oct 30, 2019

Uh oh!

jrose-apple commented Oct 30, 2019

Uh oh!

jrose-apple Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

yln Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

yln commented Oct 30, 2019

Uh oh!

gottesmm commented Oct 30, 2019 via email

Uh oh!

finagolfin commented Oct 30, 2019

Uh oh!

compnerd commented Oct 30, 2019

Uh oh!

finagolfin commented Oct 30, 2019

Uh oh!

jrose-apple commented Oct 30, 2019

Uh oh!

compnerd commented Oct 30, 2019

Uh oh!

yln commented Oct 30, 2019

Uh oh!

yln commented Oct 30, 2019

Uh oh!

yln commented Oct 30, 2019

Uh oh!

swift-ci commented Oct 30, 2019

Uh oh!

swift-ci commented Oct 30, 2019

Uh oh!

gottesmm commented Oct 31, 2019

Uh oh!

yln commented Oct 31, 2019

Uh oh!

swift-ci commented Oct 31, 2019

Uh oh!

swift-ci commented Oct 31, 2019

Uh oh!

yln commented Nov 1, 2019

Uh oh!

swift-ci commented Nov 1, 2019

Uh oh!

yln commented Nov 1, 2019

Uh oh!

yln commented Nov 11, 2019

Uh oh!