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.

Sign up for GitHub

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

Jump to bottom

Fix –-toolchain value shadowed by local executables #7483

Merged
merged 42 commits into from
Jun 6, 2024
Merged

Fix –-toolchain value shadowed by local executables #7483

merged 42 commits into from
Jun 6, 2024

Conversation

MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Apr 23, 2024 •
edited by xedin

Behavior of --toolchain regressed in https://github.com/apple/swift-package-manager/pull/7296/files by preferring the first match in toolset root paths: when swiftc is available in the same directory next to the SwiftPM binary being invoked, that became preferred with --toolchain value ignored.

This is now fixed by slightly tweaking where custom toolchain gets added - it's prepended.

A newly added test verifies that the behavior is fixed. For that we had to inject FileSystem and EnvironmentVariables in a lot more places, with an added benefit of making our tests more consistent and isolated from an arbitrary build environment.

Resolves: rdar://126095653

@MaxDesiatov
Cover –-toolchain argument in SwiftCommandStateTests
d91abb3 
We should prevent possible regressions in how this argument is handled.
@MaxDesiatov MaxDesiatov added cross-compilation test suite improvements to SwiftPM test suite build system Changes to interactions with build systems swift build Changes impacting `swift build` labels Apr 23, 2024
@MaxDesiatov MaxDesiatov self-assigned this Apr 23, 2024
@MaxDesiatov
Copy link
Member Author

@swift-ci test

MaxDesiatov added a commit that referenced this pull request Apr 23, 2024
@MaxDesiatov
Remove TSCBasic.InMemoryFileSystem dependency
314fb0a 
Since #7483 this type is implemented in SwiftPM.
@MaxDesiatov MaxDesiatov mentioned this pull request Apr 23, 2024
Draft
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov mentioned this pull request Apr 23, 2024
Draft
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov marked this pull request as draft April 23, 2024 16:20
@MaxDesiatov MaxDesiatov changed the title Cover –-toolchain argument in SwiftCommandStateTests Fix –-toolchain behavior regression May 2, 2024
@MaxDesiatov MaxDesiatov changed the title Fix –-toolchain behavior regression Fix –-toolchain value shadowed by local executables May 2, 2024
@MaxDesiatov MaxDesiatov marked this pull request as ready for review May 2, 2024 19:19
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@xedin
Copy link
Member

xedin commented May 16, 2024

@swift-ci please test

@xedin xedin mentioned this pull request May 16, 2024
Draft
@xedin
Copy link
Member

xedin commented May 16, 2024

For posterity: I changed the logic here slightly, --toolchain gets prepended (to override existing entries) but otherwise we keep the ordering of the entries in rootPaths the same which means that we are going to select a tool from the first matching root in order they were provided and the fact that we weren't doing that before seems unintentional.

@xedin
Copy link
Member

xedin commented May 17, 2024

@swift-ci please clean test Linux platform

1 similar comment
@xedin
Copy link
Member

xedin commented May 17, 2024

@swift-ci please clean test Linux platform

@xedin xedin force-pushed the maxd/test-toolchain-argument branch from 3ecfea4 to a68d7cf Compare May 17, 2024 07:25
@xedin
Copy link
Member

xedin commented May 17, 2024

@swift-ci please clean test Linux platform

xedin added 2 commits May 17, 2024 10:13
@xedin
Revert "[CoreCommands] Pipe environment through to the host toolchain"
dfa56d3 
This reverts commit 133fbee.
@xedin
[Tests] NFC: Improve SwiftCommandStateTests.testToolchaArgument
a7f8121 
Drop `SWIFTPM_CUSTOM_BIN_DIR` and related files because they are
not passed through to the `hostSwiftSDK` anyway and even if they
were we cannot use non-existant binaries because `targetTriple`
of host toolchain gets computed by invoking `swiftc -print-target-info`.
@xedin xedin force-pushed the maxd/test-toolchain-argument branch from a68d7cf to a7f8121 Compare May 17, 2024 17:44
@xedin
Copy link
Member

xedin commented May 17, 2024

@swift-ci please clean test Linux platform

@xedin
Copy link
Member

xedin commented May 18, 2024

For posterity there are currently multiple issues here

  • _hostToolchain doesn't use environment and fileSystem provided instead if fallback to .process() and localFileSystem which cannot accommodate overrides that are only available in-memory
  • Changing only environment runs into issues with SWIFTPM_CUSTOM_BIN_DIR
  • Changing both environment and fileSystem results in a few problems on Linux:
    • CI specifies SWIFT_EXEC* which wouldn't be available in in-memory filesystem
    • Dropping that crashes in UserToolchain.init when it tries to determine targetTriple for host SDK by running /swiftpm/bin/swiftc -print-target-info

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented May 18, 2024 •
edited

  • Dropping that crashes in UserToolchain.init when it tries to determine targetTriple for host SDK by running /swiftpm/bin/swiftc -print-target-info

I worked on this one specifically in ce70582, 1f2a1ca and a few other recent commits. When specifying a fake environment with in-memory FS one should provide a predetermined host triple to avoid trying to spawn that process at a fake path.

@xedin
Copy link
Member

xedin commented May 18, 2024

@MaxDesiatov This cannot be done at the moment for SwiftCommandState which is under test, I should have mentioned explicitly that the only failing test on Linux is the new one in SwiftCommandStateTests.

We need to figure out whether we want to mock a host toolchain there, provide a host triple somehow or do something else…

@MaxDesiatov
Merge branch 'main' of github.com:apple/swift-package-manager into ma…
ad05fcb 
…xd/test-toolchain-argument

# Conflicts:
#	Tests/BuildTests/BuildPlanTests.swift
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov force-pushed the maxd/test-toolchain-argument branch from 72a9833 to ad05fcb Compare June 4, 2024 16:32
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov requested a review from xedin June 5, 2024 16:47
@MaxDesiatov
Copy link
Member Author

@swift-ci test linux

@MaxDesiatov
Copy link
Member Author

@swift-ci test macos

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

xedin
xedin reviewed Jun 5, 2024
View reviewed changes
Sources/CoreCommands/SwiftCommandState.swift
environment: self.environment,
observabilityScope: self.observabilityScope
)
hostSwiftSDK.targetTriple = self.hostTriple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR - maybe we should rename this to triple?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On SwiftSDK or on SwiftCommandState?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SwiftSDK

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SwiftSDKs have both triples specified on purpose, as one can have a Swift SDK installed supporting certain host triples that don't include the triple of a machine it's installed on. hostTriple is optional on it for Swift SDKs that don't have such requirements. But we still need to distinguish target from host there.

@MaxDesiatov MaxDesiatov merged commit ee9d0b2 into main Jun 6, 2024
5 checks passed
@MaxDesiatov MaxDesiatov deleted the maxd/test-toolchain-argument branch June 6, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin left review comments

@bnbarham bnbarham bnbarham approved these changes

@tomerd tomerd Awaiting requested review from tomerd

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@francescomikulis francescomikulis Awaiting requested review from francescomikulis francescomikulis is a code owner

Assignees

@MaxDesiatov MaxDesiatov

Labels
build system Changes to interactions with build systems command-line interface cross-compilation swift build Changes impacting `swift build` test suite improvements to SwiftPM test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MaxDesiatov @xedin @bnbarham