Set default linker to lld for Amazon Linux 2023 #72049

futurejones · 2024-03-03T00:46:46Z

The gold linker is no longer available in Amazon Linux 2023 and requires lld to be set as the default linker.

This removes the need to build the gold linker from source as proposed here - swiftlang/swift-installer-scripts#278

These changes only effect amazonlinux 2023 and should not impact on the current builds of amazonlinux 2 .

futurejones · 2024-03-03T00:51:41Z

@sebsto, can you look at this and give some feedback please.

lib/Driver/UnixToolChains.cpp

finagolfin · 2024-03-03T07:43:38Z

CMake approach seems fine if it works, but note that you will need a swift-driver pull also.

futurejones · 2024-03-03T07:55:46Z

@finagolfin The swift-driver issue has already been fixed with swiftlang/swift-driver#1545

sebsto · 2024-03-03T10:21:50Z

@sebsto, can you look at this and give some feedback please.

Thank you for working on this ! The maintainers of this project will be more qualified than me to suggest the correct approach :-)
As there is no decision taken to use lld as default linker, I would favor the approach suggested by finagolfin as it seems a better longer term decision.

etcwilde

Don't love it, but we're expecting to delete it so I'll be less picky. I would still get @artemcm's approval though. I'll land swiftlang/swift-driver#1545 once we know what our story is with lld and the removal of the start/stop symbols. I definitely prefer one of @al45tair (#71373 (comment)), @keith (#67476), or @kateinoigakukun's (#71373) approach over mine in swiftlang/llvm-project#8214.

etcwilde · 2024-03-04T05:07:24Z

lib/Driver/UnixToolChains.cpp

@@ -9,6 +9,8 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+#include <iostream>


If possible, please use the LLVM APIs available. We're already including them.

@etcwilde, I have removed #include <iostream> as it was only being used for print some log messages while I was debugging.

al45tair · 2024-03-04T09:25:40Z

I definitely prefer one of @al45tair (#71373 (comment)), @keith (#67476), or @kateinoigakukun's (#71373) approach over mine in apple/llvm-project#8214.

FWIW, I'm working on a more complete fix for that at the moment. // @etcwilde

futurejones · 2024-04-05T03:28:02Z

@al45tair @artemcm @sebsto @etcwilde @tkremenek
Is this moving forward? Can I get some indication as to whether this is going to get approved and merged or has it been abandoned in favor of another option?
Thanks.

etcwilde · 2024-04-05T04:04:23Z

Sorry, I was thinking of the other PR for changing how the driver selects the linker.

I'm definitely not a huge fan of checking the distro names, that doesn't scale and seems brittle. It's for the old driver and I don't have a better idea though. Given that this is @artemcm's world, I defer final vote to him. Alternatively, @al45tair might have an idea for checking distribution info that doesn't involve hardcoding names into the driver.

al45tair · 2024-04-05T08:51:37Z

A possible alternative might be to actually check for the existence of ld.gold and ld.lld and fall back if the one we would normally pick isn't available but the other one is. That might be better in some sense, but I think we'd have to search PATH for them, which is vastly more complicated, and this being the old driver (which is going to go away at some point), honestly I'd be inclined to merge this as-is. @artemcm what do you think?

al45tair · 2024-04-05T08:52:13Z

@swift-ci Please smoke test

al45tair · 2024-04-05T09:33:12Z

@swift-ci Please smoke test Windows platform

drodriguez · 2024-04-10T15:33:32Z

CMakeLists.txt

@@ -948,6 +948,10 @@ if(XCODE)
  swift_common_xcode_cxx_config()
 endif()

+# Check what linux distribution is being used.
+# This can be used to determine the default linker to use.
+cmake_host_system_information(RESULT DISTRO_NAME  QUERY DISTRIB_PRETTY_NAME)


This should be reverted.

The minimum CMake version is 3.19.6 (says so at the top of the file).

This key only appeared on CMake 3.22.

@drodriguez @finagolfin @etcwilde
The minimum cmake version required for the swift toolchain build has been "cmake": "v3.24.2" since version 5.10

https://github.com/apple/swift/blob/main/utils/update_checkout/update-checkout-config.json#L232

#67018

That's the version used by the build-script for CI, and only on Linux and BSD. The CMake files should be compatible with the version required by the cmake_minimum_required instruction. This is failing for a macOS which a 3.20 version of CMake (enough to build LLVM and Swift using only CMake).

I think the best option here is to update the minimum cmake version to match the version used in the CI.
This was supposed to have been done already and it makes no sense to be using a cmake version as old as v3.19.6

I think @etcwilde plans to significantly increase the minimum CMake version this year anyway, so it would make sense to increase it to 3.22 now itself.

Evan, wdyt?

But things need to be done in certain order, and not as a reaction to introducing a dependency to check for a specific Linux distribution, and have a broken CMakeLists.txt at the root of the project.

Assuming #73001 fixes the problem for now, we can discuss the CMake version separately.

One CMake version bump that should be very easy to justify is to 3.20.0, since that's what LLVM needs, and anybody trying to build would need at least that.

Going beyond that is probably a question of distribution support. macOS is an outlier: if one uses Homebrew you are fine, but if some other packing system is used, people will need to upgrade. I think the problem in Linux and BSD distros is worked around with the compiled CMake version (at least when using build-script).

IMO, for any platform, in any case, it would be good if the actual needed CMake version was detailed in the main CMakeLists.txt file.

drodriguez · 2024-04-10T15:53:00Z

I think the feelings above about hardcoding distribution names were correct. This could have been a configuration in a cache file used to build for that particular distribution. If some particular distribution needs an override for the default linker chosen by the toolchain, that can also be injected during the configuration. It does not need to be hardcoded into the build system (or the compiler).

finagolfin · 2024-04-10T16:08:22Z

I think we lowered our standards, as stated above, because the real fix is in swiftlang/swift-driver#1545 and this legacy Driver is slated to be deleted soon.

etcwilde · 2024-04-10T16:09:52Z

We still need something that is valid for the stated version of CMake. I'm not personally opposed to updating the CMake requirements, but that should come from an explicit decision to do so.

finagolfin · 2024-04-10T16:46:17Z

@futurejones, would you submit a revision to the CMake change that will work with older CMake also?

al45tair · 2024-04-12T08:57:34Z

drodriguez:
I think the feelings above about hardcoding distribution names were correct.

finagolfin:
I think we lowered our standards, as stated above, because the real fix is in swiftlang/swift-driver#1545 and this legacy Driver is slated to be deleted soon.

Exactly this. Hardcoding distribution names isn't great, but it lets us make forward progress and we have a better fix in the works already.

The CMake version problem is a bigger issue, mind. That said, I think we could just do

if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.22")
  cmake_host_system_information(RESULT DISTRO_NAME  QUERY DISTRIB_PRETTY_NAME)
endif()

and tell people using Amazon Linux 2023 that they need at least CMake 3.22 for things to work. @drodriguez since you have an old CMake installed, could you confirm that that change fixes things for you? I've raised a PR for the change as #73001 and added you as a reviewer.

drodriguez:
This could have been a configuration in a cache file used to build for that particular distribution.

If we're talking about official distributions of Swift, either provided by swift.org or by the Amazon Linux folks, sure. I'm not sure that's such a great solution in general though — it'd mean having to have special instructions for building on Amazon Linux 2023 and above. And we already have a better solution in swiftlang/swift-driver#1545.

futurejones · 2024-04-12T09:48:11Z

having to have special instructions for building on Amazon Linux 2023

There should be no need for any special instructions for Amazon Linux 2023 as the default install version of CMake is already v3.22.2

sebsto · 2024-09-28T11:06:18Z

@futurejones @al45tair I managed to build Swift 6 on ALI2023 thanks to your PRs.

The trick is to install ld.gold and Swift 5.10 on the build machine to create the tarball.
See the script here https://gist.github.com/sebsto/6409ed9dbf4c9dd1488e049165f751aa

However, when I try to use the tarball I created on a new ALI2023 machine, the swift driver (or compiler) still expects to find ld.gold
I thought this PR would cause the Swift 6.x driver to pickup lld by default. See below.

[ec2-user@ip-172-31-25-1 test]$ swift --version 
Swift version 6.0.1-dev (LLVM 73500bf55acff5f, Swift 1e770fa56a3fff2)
Target: x86_64-unknown-linux-gnu

[ec2-user@ip-172-31-25-1 test]$ swift package init --type executable 
Creating executable package: test
Creating Package.swift
Creating .gitignore
Creating Sources/
Creating Sources/main.swift

[ec2-user@ip-172-31-25-1 test]$ swift build
error: 'test': Invalid manifest (compiled with: ["/home/ec2-user/swift/usr/bin/swiftc", "-vfsoverlay", "/tmp/TemporaryDirectory.tlr5uA/vfs.yaml", "-L", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-lPackageDescription", "-Xlinker", "-rpath", "-Xlinker", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-swift-version", "6", "-I", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-package-description-version", "6.0.0", "/home/ec2-user/test/Package.swift", "-o", "/tmp/TemporaryDirectory.6Jg8Bw/test-manifest"])
error: link command failed with exit code 1 (use -v to see invocation)
clang: error: invalid linker name in argument '-fuse-ld=gold'
error: 'test': Invalid manifest (compiled with: ["/home/ec2-user/swift/usr/bin/swiftc", "-vfsoverlay", "/tmp/TemporaryDirectory.4iTMeI/vfs.yaml", "-L", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-lPackageDescription", "-Xlinker", "-rpath", "-Xlinker", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-swift-version", "6", "-I", "/home/ec2-user/swift/usr/lib/swift/pm/ManifestAPI", "-package-description-version", "6.0.0", "/home/ec2-user/test/Package.swift", "-o", "/tmp/TemporaryDirectory.CW8Y5W/test-manifest"])
error: link command failed with exit code 1 (use -v to see invocation)
clang: error: invalid linker name in argument '-fuse-ld=gold'

What did I do wrong ?

Is the change not merged yet on the 6.x branches ? (Just found out swiftlang/swift-driver#1595)

finagolfin · 2024-09-28T12:02:36Z

Is the change not merged yet on the 6.x branches ? (Just found out swiftlang/swift-driver#1595)

As I noted six months ago, swift-driver is where this is really set. If you pull in that linked pull, it should work.

sebsto · 2024-09-28T17:04:02Z

@finagolfin is this the one you refer too ?
swiftlang/swift-driver#1545

This has been merged into main but is not in 6.0.1 nor 6.0.2 :-(

finagolfin · 2024-09-28T17:12:15Z

Yes, it was submitted to 6.0 a while back in swiftlang/swift-driver#1595, which you originally linked. Evan says there that that pull also needs another one from main, so try pulling those two in to your build and see if it works.

futurejones requested a review from artemcm as a code owner March 3, 2024 00:46

finagolfin reviewed Mar 3, 2024

View reviewed changes

lib/Driver/UnixToolChains.cpp Outdated Show resolved Hide resolved

MaxDesiatov requested review from etcwilde and al45tair March 3, 2024 08:51

MaxDesiatov added Linux Platform: Linux linker error labels Mar 3, 2024

etcwilde reviewed Mar 4, 2024

View reviewed changes

set default linker to lld for Amazon Linux 2023

03a364e

futurejones force-pushed the amazonlinux-use-lld-linker branch from 614ba0a to 03a364e Compare March 5, 2024 02:19

artemcm approved these changes Apr 9, 2024

View reviewed changes

al45tair merged commit e112478 into swiftlang:main Apr 10, 2024
3 checks passed

drodriguez reviewed Apr 10, 2024

View reviewed changes

al45tair mentioned this pull request Apr 12, 2024

[Build] Fix host system query for older CMake. #73001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default linker to lld for Amazon Linux 2023 #72049

Set default linker to lld for Amazon Linux 2023 #72049

futurejones commented Mar 3, 2024

futurejones commented Mar 3, 2024

finagolfin commented Mar 3, 2024

futurejones commented Mar 3, 2024

sebsto commented Mar 3, 2024

etcwilde left a comment

etcwilde Mar 4, 2024

futurejones Mar 5, 2024

al45tair commented Mar 4, 2024 •

edited

Loading

futurejones commented Apr 5, 2024

etcwilde commented Apr 5, 2024

al45tair commented Apr 5, 2024

al45tair commented Apr 5, 2024

al45tair commented Apr 5, 2024

drodriguez Apr 10, 2024

futurejones Apr 11, 2024 •

edited

Loading

drodriguez Apr 11, 2024

futurejones Apr 11, 2024

finagolfin Apr 11, 2024

drodriguez Apr 11, 2024

al45tair Apr 12, 2024

drodriguez Apr 12, 2024

drodriguez commented Apr 10, 2024

finagolfin commented Apr 10, 2024

etcwilde commented Apr 10, 2024

finagolfin commented Apr 10, 2024

al45tair commented Apr 12, 2024 •

edited

Loading

futurejones commented Apr 12, 2024

sebsto commented Sep 28, 2024 •

edited

Loading

finagolfin commented Sep 28, 2024

sebsto commented Sep 28, 2024

finagolfin commented Sep 28, 2024 •

edited

Loading

Set default linker to lld for Amazon Linux 2023 #72049

Set default linker to lld for Amazon Linux 2023 #72049

Conversation

futurejones commented Mar 3, 2024

futurejones commented Mar 3, 2024

finagolfin commented Mar 3, 2024

futurejones commented Mar 3, 2024

sebsto commented Mar 3, 2024

etcwilde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al45tair commented Mar 4, 2024 • edited Loading

futurejones commented Apr 5, 2024

etcwilde commented Apr 5, 2024

al45tair commented Apr 5, 2024

al45tair commented Apr 5, 2024

al45tair commented Apr 5, 2024

Choose a reason for hiding this comment

futurejones Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodriguez commented Apr 10, 2024

finagolfin commented Apr 10, 2024

etcwilde commented Apr 10, 2024

finagolfin commented Apr 10, 2024

al45tair commented Apr 12, 2024 • edited Loading

futurejones commented Apr 12, 2024

sebsto commented Sep 28, 2024 • edited Loading

finagolfin commented Sep 28, 2024

sebsto commented Sep 28, 2024

finagolfin commented Sep 28, 2024 • edited Loading

al45tair commented Mar 4, 2024 •

edited

Loading

futurejones Apr 11, 2024 •

edited

Loading

al45tair commented Apr 12, 2024 •

edited

Loading

sebsto commented Sep 28, 2024 •

edited

Loading

finagolfin commented Sep 28, 2024 •

edited

Loading