Skip to content
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

Don't add an autolink-extract job unless actually linking ELF/Wasm objects, matching the original C++ driver #1479

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

finagolfin
Copy link
Contributor

This is needed for swiftlang/swift#69564, which enables building the early swift driver and running tests from the compiler validation suite with it for the first time on the linux CI. Looking at those test failures, I noticed that the Swift toolchain wrongly runs autolink-extract on linux when simply compiling object files, which is obviously unused:

> ./swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swiftc -c swift/test/Interpreter/hello_toplevel.swift -v
Swift version 5.11-dev (LLVM e22c96610989bdb, Swift bd372c2d6861ce3)
Target: x86_64-unknown-linux-gnu
/home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swift-frontend -frontend -c -primary-file swift/test/Interpreter/hello_toplevel.swift -target x86_64-unknown-linux-gnu -disable-objc-interop -color-diagnostics -new-driver-path /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swift-driver -empty-abi-descriptor -resource-dir /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/lib/swift -module-name hello_toplevel -plugin-path /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/lib/swift/host/plugins -plugin-path /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/local/lib/swift/host/plugins -o hello_toplevel.o
/home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swift-autolink-extract hello_toplevel.o -o /tmp/TemporaryDirectory.3RqPzu/hello_toplevel-1.autolink

and which the old C++ driver doesn't:

> ./swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swiftc -c swift/test/Interpreter/hello_toplevel.swift -disallow-use-new-driver -v
<unknown>:0: warning: legacy driver is now deprecated; consider avoiding specifying '-disallow-use-new-driver'
Swift version 5.11-dev (LLVM e22c96610989bdb, Swift bd372c2d6861ce3)
Target: x86_64-unknown-linux-gnu
/home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/bin/swift-frontend -frontend -c -primary-file swift/test/Interpreter/hello_toplevel.swift -target x86_64-unknown-linux-gnu -disable-objc-interop -color-diagnostics -plugin-path /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/lib/swift/host/plugins -plugin-path /home/foo/swift-DEVELOPMENT-SNAPSHOT-2023-10-30-a-ubuntu20.04/usr/local/lib/swift/host/plugins -module-name hello_toplevel -o hello_toplevel.o

That's because the old driver checks if it shouldLink() before scheduling an autolink-extract, whereas there's no equivalent check in this repo going back to when the autolink-extract job was added in c4bf245. That's why @eeckstein had to add this linux-specific hack for these tests he added in #1019, which can now be removed and acts as a regression test.

The autolink-extract could also be disabled for static libraries, but I kept the behavior the same across both drivers instead.

@MaxDesiatov MaxDesiatov changed the title Don't add an autolink-extract job unless actually linking ELF/WASM objects, matching the original C++ driver Don't add an autolink-extract job unless actually linking ELF/Wasm objects, matching the original C++ driver Nov 5, 2023
@finagolfin
Copy link
Contributor Author

@bnbarham, need a CI run here.

@artemcm
Copy link
Contributor

artemcm commented Nov 6, 2023

@swift-ci test

@finagolfin
Copy link
Contributor Author

Mac and linux CI passed; Windows CI appears to be broken right now, but doesn't matter because this pull changes nothing for Windows.

@artemcm
Copy link
Contributor

artemcm commented Nov 6, 2023

@swift-ci test Windows platform

@finagolfin
Copy link
Contributor Author

finagolfin commented Nov 7, 2023

Windows CI failed again with the same error unrelated to this pull.

@finagolfin
Copy link
Contributor Author

Looks like the Windows CI is passing again on the main compiler repo, one more Windows run here now and it should pass, @artemcm.

@artemcm
Copy link
Contributor

artemcm commented Nov 9, 2023

@swift-ci test Windows platform

@finagolfin
Copy link
Contributor Author

@compnerd, the Windows CI is repeatedly failing on this repo when installing ICU, unrelated to these driver pulls:

Copy-Item : Could not find a part of the path 'C:\Users\swift-ci\jenkins\workspace\swift-driver-PR-windows\icu\icu4c\'.
At C:\Users\swift-ci\jenkins\workspace\swift-driver-PR-windows\swift\utils\build.ps1:990 char:7
+       Copy-Item $SourceCache\swift-installer-scripts\shared\ICU\CMake ...
+       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Copy-Item], DirectoryNotFoundException
    + FullyQualifiedErrorId : System.IO.DirectoryNotFoundException,Microsoft.PowerShell.Commands.CopyItemCommand

Does something need to be adjusted on this CI?

@compnerd
Copy link
Member

compnerd commented Nov 9, 2023

@finagolfin could you please file an issue on apple/swift and include @shahmishal? I think that there were some tweaks that were needed as old revisions were being checked out.

@shahmishal
Copy link
Member

@swift-ci test Windows platform

@bnbarham bnbarham merged commit fdae36f into swiftlang:main Nov 9, 2023
3 checks passed
@finagolfin finagolfin deleted the autolink branch November 10, 2023 03: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.

5 participants