Implement `swift-get-version` in SwiftPM #6840

neonichu · 2023-08-23T00:35:02Z

The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of swift -version for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018

neonichu · 2023-08-23T00:35:15Z

@swift-ci please smoke test

neonichu · 2023-08-23T01:30:50Z

Linux smoke test failure is a compiler issue:

/home/build-user/build/buildbot_incremental/swift-linux-x86_64/bin/swiftc: symbol lookup error: /home/build-user/build/buildbot_incremental/swift-linux-x86_64/bin/swiftc: undefined symbol: swift_getTypeByMangledNameInContext2

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

neonichu · 2023-08-23T03:21:28Z

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

            return []
        }
-        guard let tool = buildDescription.copyCommands[command.name] else {
+        guard let tool = buildDescription.writeCommands[command.name] else {
+            self.context.observabilityScope.emit(error: "command \(command.name) not registered")


This was previously completely wrong and was always returning an empty signature.

Tests/CommandsTests/BuildToolTests.swift

finagolfin · 2023-08-23T13:13:51Z

Linux smoke test failure is a compiler issue

That error means the freshly-built trunk compiler is dynamically linking against the 5.8 Swift stdlib somehow, though I'm not sure how that's possible unless you're modifying LD_LIBRARY_PATH somewhere to add the path to that 5.8 stdlib.

I know this because I had to fix it when compiling the Aug. 10 trunk source snapshot natively on Android recently, because some test runners and SwiftPM were actually dynamically linked against the Swift 5.8 stdlib, then failing because it doesn't have that swift_getTypeByMangledNameInContext2 symbol that was just added to the trunk 5.10 stdlib last month in swiftlang/swift#67168.

Note that Swift 5.8.1 was just pre-installed to the linux CI yesterday, swiftlang/swift-docker#346. Pinging @bnbarham, since you're dealing with installing a prebuilt release toolchain to the linux CI.

bnbarham · 2023-08-23T17:44:59Z

It's the same issue as your other PR. The PR I linked there will fix it, sorry for the noise.

finagolfin · 2023-08-23T18:02:38Z

It's the same issue as your other PR. The PR I linked there will fix it, sorry for the noise.

No, my 5.9 pull has a different error when linking the swift-compatibility-symbols executable:

/usr/bin/ld.gold: error: cannot find -lswiftCore
stdlib/public/runtime/CMakeFiles/swiftImageRegistrationObjectELF-linux-x86_64.dir/SwiftRT-ELF.cpp.o:SwiftRT-ELF.cpp:function swift_image_constructor(): error: undefined reference to 'swift_addNewDSOImage'

Looking into this pull's build log further, it is failing in the compiler build when building a backtrace file, which is quite strange, since it's able to build other stdlib files fine. I notice an env LD_LIBRARY_PATH=/home/build-user/build/buildbot_incremental/swift-linux-x86_64/bootstrapping1/lib/swift/linux for those other files, which appears to be set here if IS_STDLIB is set, but the new swift-backtrace executable doesn't set that, even though it is built alongside the stdlib.

This probably worked before because the runpath is already set for the freshly-built trunk Swift compiler to use the freshly-built stdlib, but now it is somehow picking up the 5.8 stdlib instead? I'm still not sure where it's getting the path to the 5.8 stdlib.

bnbarham · 2023-08-23T18:17:54Z

It's a different error, but the underlying cause is the similar - we hit this on other jobs. If my PR doesn't fix this I'll look into it further.

finagolfin · 2023-08-23T18:20:59Z

It's a different error, but the underlying cause is the similar - we hit this on other jobs. If my PR doesn't fix this I'll look into it further.

I'm fairly certain disabling building early swift driver/syntax won't fix this backtrace build error seen here, but I'm happy to be proven wrong. Let's see.

neonichu · 2023-08-23T20:01:41Z

@swift-ci please smoke test linux

neonichu · 2023-08-23T20:46:00Z

swiftlang/swift#68094
@swift-ci please smoke test

neonichu · 2023-08-23T20:46:16Z

@swift-ci please test windows

finagolfin · 2023-08-23T20:50:57Z

swiftlang/swift#68094 is for 5.9, not sure if it will work with this trunk pull, you want swiftlang/swift#68091.

neonichu · 2023-08-23T21:06:54Z

swiftlang/swift#68091
@swift-ci please smoke test

neonichu · 2023-08-23T21:07:01Z

@swift-ci please test windows

neonichu · 2023-08-23T21:13:03Z

swiftlang/swift#68091
@swift-ci please smoke test

neonichu · 2023-08-23T21:13:10Z

@swift-ci please test windows

finagolfin · 2023-08-23T21:22:18Z

Looks like your new test is failing on the self-hosted linux run, putting aside the upstream issues with the smoke test.

neonichu · 2023-08-23T21:26:13Z

Yep, this looks like a real issue with my PR

neonichu · 2023-08-23T21:39:03Z

I think the issue is that the script I am using in the test doesn't work on Linux as is:

root@4efefb880470:~/swift-package-manager# ./Fixtures/Scripts/dummy-swiftc.sh -version
./Fixtures/Scripts/dummy-swiftc.sh: 5: [: -version: unexpected operator
Swift version 5.8.1 (swift-5.8.1-RELEASE)
Target: aarch64-unknown-linux-gnu

neonichu · 2023-08-23T23:35:32Z

swiftlang/swift#68091
@swift-ci please smoke test

neonichu · 2023-08-23T23:35:40Z

@swift-ci please test windows

neonichu · 2023-08-23T23:35:52Z

@swift-ci please test windows

neonichu · 2023-08-23T23:36:09Z

@swift-ci please test windows

neonichu · 2023-08-23T23:36:22Z

@swift-ci please smoke test windows

neonichu · 2023-08-23T23:36:35Z

@swift-ci please test windows

neonichu · 2023-08-23T23:36:54Z

¯\_(ツ)_/¯

neonichu · 2023-08-23T23:43:32Z

@swift-ci please test windows

neonichu · 2023-08-24T00:11:46Z

@swift-ci please test windows

neonichu · 2023-08-24T00:14:56Z

Seems like Windows CI is just broken now

Running ``update_single_repository`` with up to 72 processes.
======UPDATE FAILURES======
C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift failed (ret=1): ['git', 'checkout', 'main']
b"error: pathspec 'main' did not match any file(s) known to git\n"
update-checkout failed, fix errors and try again
Build step 'Execute Windows batch command' marked build as failure

cc @shahmishal

Sources/Build/LLBuildManifestBuilder.swift

tomerd

couple of small suggestions / questions

neonichu · 2023-08-24T00:45:59Z

macOS smoke test failure

+ git branch -D ci_pr_68091
error: branch 'ci_pr_68091' not found.
+ git fetch origin pull/68091/merge:ci_pr_68091 --tags
b"fatal: couldn't find remote ref pull/68091/merge"
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swift/utils/update-checkout", line 15, in <module>
    update_checkout.main()
  File "/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swift/utils/update_checkout/update_checkout/update_checkout.py", line 707, in main
    branch_name, cross_repo = get_branch_for_repo(config, 'swift',
  File "/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swift/utils/update_checkout/update_checkout/update_checkout.py", line 143, in get_branch_for_repo
    shell.run(["git", "fetch", "origin",
  File "/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swift/utils/swift_build_support/swift_build_support/shell.py", line 257, in run
    raise eout
Exception: ['git', 'fetch', 'origin', 'pull/68091/merge:ci_pr_68091', '--tags']

I guess the PR has been merged since I kicked off the build. A bit unfortunate that we essentially have a multi-hour race condition window?

bnbarham · 2023-08-24T00:57:17Z

I guess the PR has been merged since I kicked off the build. A bit unfortunate that we essentially have a multi-hour race condition window?

I'm so sorry :(. All the PRs are merged now

finagolfin · 2023-08-24T03:14:09Z

I'm fairly certain disabling building early swift driver/syntax won't fix this backtrace build error seen here, but I'm happy to be proven wrong.

You were right, @bnbarham, disabling those other products fixed this build. Oh well, something to watch out for when you enable them again, I think IS_STDLIB or something similar will need to be added for swift-backtrace then.

neonichu · 2023-08-24T03:29:53Z

Feels weird that self-hosted is also affected by merging a PR, even though it doesn't even support cross-repo testing at all. With macOS hosts always pretty backed up, it might be nice to just fall back to main if a PR has been merged?

neonichu · 2023-08-24T04:42:59Z

@swift-ci please smoke test macOS

neonichu · 2023-08-24T04:43:28Z

@swift-ci please test windows

compnerd

Please mark the test to be skipped on all platforms but Darwin. The introduction of bash means that the test is not portable. (Debian doesn't default to including bash, it uses dash as a portable ash, windows does not have bash either).

neonichu · 2023-08-24T16:36:27Z

Debian is not a supported platform, skipping the test for Windows seems fine, but I'd like to do this in a follow up since we really should be fixing this regression.

neonichu · 2023-08-24T16:45:21Z

Debian is not a supported platform, skipping the test for Windows seems fine, but I'd like to do this in a follow up since we really should be fixing this regression.

Actually looks like we did make any existing tests using bash scripts macOS only, so makes sense to do it for this as well. Still don't agree that this is a merge blocker, we are not running these tests on any affected platforms AFAIK.

neonichu · 2023-08-24T21:26:19Z

Wondering if I just take a different approach from a bash script and just write a small Swift program which can be a dependency of this test. That way it could hopefully be more portable.

The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that. We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes. rdar://114047018

neonichu · 2023-08-25T02:33:14Z

@swift-ci please smoke test

neonichu · 2023-08-25T02:33:26Z

@swift-ci please test windows

Sources/dummy-swiftc/main.swift

neonichu self-assigned this Aug 23, 2023

neonichu force-pushed the swift-get-version branch from 8be50bf to c14ce02 Compare August 23, 2023 03:19

neonichu changed the title ~~WIP: swift-get-version~~ WIP: Implement swift-get-version in SwiftPM Aug 23, 2023

neonichu commented Aug 23, 2023

View reviewed changes

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift Outdated Show resolved Hide resolved

neonichu commented Aug 23, 2023

View reviewed changes

Tests/CommandsTests/BuildToolTests.swift Outdated Show resolved Hide resolved

neonichu force-pushed the swift-get-version branch from c14ce02 to 2b35cad Compare August 23, 2023 20:28

neonichu changed the title ~~WIP: Implement swift-get-version in SwiftPM~~ Implement swift-get-version in SwiftPM Aug 23, 2023

neonichu marked this pull request as ready for review August 23, 2023 20:44

neonichu requested review from abertelrud, tomerd and MaxDesiatov as code owners August 23, 2023 20:44

tomerd reviewed Aug 24, 2023

View reviewed changes

Sources/Build/LLBuildManifestBuilder.swift Show resolved Hide resolved

tomerd reviewed Aug 24, 2023

View reviewed changes

Sources/Build/LLBuildManifestBuilder.swift Show resolved Hide resolved

tomerd approved these changes Aug 24, 2023

View reviewed changes

compnerd requested changes Aug 24, 2023

View reviewed changes

neonichu force-pushed the swift-get-version branch from d9b8920 to 5f48133 Compare August 25, 2023 02:32

compnerd approved these changes Aug 25, 2023

View reviewed changes

Sources/dummy-swiftc/main.swift Show resolved Hide resolved

neonichu merged commit 55936dc into main Aug 25, 2023

neonichu deleted the swift-get-version branch August 25, 2023 15:58

Implement swift-get-version in SwiftPM #6840

Implement swift-get-version in SwiftPM #6840

Conversation

neonichu commented Aug 23, 2023 • edited Loading

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu Aug 23, 2023

Choose a reason for hiding this comment

finagolfin commented Aug 23, 2023 • edited Loading

bnbarham commented Aug 23, 2023

finagolfin commented Aug 23, 2023 • edited Loading

bnbarham commented Aug 23, 2023

finagolfin commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

finagolfin commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

finagolfin commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 23, 2023

neonichu commented Aug 24, 2023

neonichu commented Aug 24, 2023

tomerd left a comment

Choose a reason for hiding this comment

neonichu commented Aug 24, 2023

bnbarham commented Aug 24, 2023

finagolfin commented Aug 24, 2023

neonichu commented Aug 24, 2023

neonichu commented Aug 24, 2023

neonichu commented Aug 24, 2023

compnerd left a comment

Choose a reason for hiding this comment

neonichu commented Aug 24, 2023

neonichu commented Aug 24, 2023 • edited Loading

neonichu commented Aug 24, 2023

neonichu commented Aug 25, 2023

neonichu commented Aug 25, 2023

Implement `swift-get-version` in SwiftPM #6840

Implement `swift-get-version` in SwiftPM #6840

neonichu commented Aug 23, 2023 •

edited

Loading

finagolfin commented Aug 23, 2023 •

edited

Loading

finagolfin commented Aug 23, 2023 •

edited

Loading

neonichu commented Aug 24, 2023 •

edited

Loading