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

[PackageModel] Support swift-testing installed in a toolchain #7840

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jul 30, 2024

Properly handle swift-testing installations in toolchain/SDK.

Motivation:

On macOS SwiftPM should prefer swift-testing installed into a custom toolchain when used. On Windows we need special logic to discover swift-testing location.

Modifications:

  • Add special swift compiler "extra" flags to favor swift-testing installed in a toolchain.
  • Inject -I, -L on Windows that point to where swift-testing is installed in SDKROOT.
  • Inject a path to testing on PATH environment variable on Windows to make sure that the library is always discoverable.

Resolves: rdar://132828246

On MacOS toolchain can shadow SDK content. This method is intended
to locate and include swift-testing library from a toolchain before
sdk content which to sure that builds that use a custom toolchain
always get a custom swift-testing library as well.

// The layout of the SDK is as follows:
//
// Library/Developer/Platforms/[PLATFORM].platform/Developer/Library/<Project>-[VERSION]/...
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really use Library/Developer directory naming on Windows? I thought this was very Darwin-specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd think so since this was the original comment I moved, that's how we discover XCTest today.

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM with a minor nit about a Windows-specific layout comment

@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test Windows

@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test Windows platform

2 similar comments
@xedin
Copy link
Contributor Author

xedin commented Aug 2, 2024

@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Aug 3, 2024

@swift-ci please test Windows platform

xedin added 3 commits August 2, 2024 17:55
The library is going to be installed into SDK beside XCTest.
Windows requires that the library with adjacent to the executable
or we discoverable via PATH.
@xedin xedin force-pushed the swift-testing-in-toolchain branch from bdc42f9 to 47120a0 Compare August 3, 2024 00:55
@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

Obviously cross repo PR testing from non-swift repo doesn't support changes in update-checkout scheme.
@swift-ci Please test

@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

@swift-ci Please test

@rintaro
Copy link
Member

rintaro commented Aug 3, 2024

Windows test fails presumably because it fails to decode Info.plist. Since swiftTestingVersion is not optional, it requires corresponding swift change.

@xedin
Copy link
Contributor Author

xedin commented Aug 3, 2024

@rintaro maybe it would be prudent to make it optional just in case people would try to run newer swiftpm with older toolchains?

@rintaro
Copy link
Member

rintaro commented Aug 4, 2024

Yeah, optional sounds reasonable to me.

@xedin
Copy link
Contributor Author

xedin commented Aug 4, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Aug 4, 2024

@swift-ci please test Windows platform

@xedin xedin force-pushed the swift-testing-in-toolchain branch from 53b192e to a116a47 Compare August 4, 2024 15:13
@xedin
Copy link
Contributor Author

xedin commented Aug 4, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Aug 4, 2024

@swift-ci please test Windows platform

xedin added 2 commits August 5, 2024 09:22
…ironment

Replace all of the copies of SDKROOT handling with a single property
access on Environment type.
This allows us to support older Windows toolchains with newer
swift package manager even the project only uses XCTest.
@xedin xedin force-pushed the swift-testing-in-toolchain branch from a116a47 to 5b2e5d2 Compare August 5, 2024 16:28
@xedin
Copy link
Contributor Author

xedin commented Aug 5, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Aug 5, 2024

@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Aug 5, 2024

I'm going to land this is as a rebase because it has changes for both macOS and Windows and we should be revertible separately if we wanted to.

@xedin xedin merged commit fbf87bf into swiftlang:main Aug 5, 2024
5 checks passed
return try AbsolutePath(validating: "../../lib/swift/macosx", relativeTo: resolveSymlinks(swiftCompilerPath))
try Self.toolchainLibDir(swiftCompilerPath: self.swiftCompilerPath).appending(
components: ["swift", "macosx"]
)
}
}

public var toolchainLibDir: AbsolutePath {
get throws {
// FIXME: Not sure if it's better to base this off of Swift compiler or our own binary.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME: Not sure if it's better to base this off of Swift compiler or our own binary.

extraSwiftCFlags = info.defaults.extraSwiftCFlags ?? []
}

return ["-sdk", sdkroot.pathString] + runtime + xctest + extraSwiftCFlags
return ["-sdk", sdkroot.pathString] + runtime + xctest + swiftTesting + extraSwiftCFlags
Copy link
Member

Choose a reason for hiding this comment

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

Can we limit to the testing framework in use by any chance?

xedin added a commit that referenced this pull request Aug 13, 2024
…7856)

- Explanation:

On macOS SwiftPM should prefer swift-testing installed into a custom
toolchain when used. On Windows we need special logic to discover
swift-testing location.

- Add special swift compiler "extra" flags to favor swift-testing
installed in a toolchain.
- Inject `-I`, `-L` on Windows that point to where swift-testing is
installed in SDKROOT.
- Inject a path to testing on `PATH` environment variable on Windows to
make sure that the library is always discoverable.

- Main Branch PR:
#7840

- Resolves: rdar://132828246

- Risk: Medium (Although changes are only viable with toolchains have
certain directories in them and test we could do for testing was manual
validation).

- Reviewed By: @MaxDesiatov @rintaro 

- Testing: Existing tests and manual validation using new toolchain
(which is currently in development) on Windows and a custom toolchain
plus CommandLine tools on macOS.
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.

4 participants