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

Set -user-module-version for any version-based package #6781

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented Aug 7, 2023

This would allow conditionalizing client code using #if canImport(:_version:) so that different versions of a dependency can be supported within a single codebase.

Note: this is explicitly not passing the flag if pre-releases are in use to avoid any confusion in those cases.

rdar://107077287

@neonichu
Copy link
Contributor Author

neonichu commented Aug 7, 2023

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 7, 2023

Unsure whether we should conditionalize this based on tools-version

@tomerd
Copy link
Contributor

tomerd commented Aug 7, 2023

Unsure whether we should conditionalize this based on tools-version

probably should for unintended consequences? or is it inert?

also should we add a test?

@neonichu
Copy link
Contributor Author

neonichu commented Aug 7, 2023

probably should for unintended consequences? or is it inert?

What is was thinking is that not tying it to the tools version would allow folks to support multiple versions if their users are using a newer SwiftPM, but they would still have support for a singular older version for users of older SwiftPMs.

For example, in the case of a Swift macro, I could write something like:

#if canImport(SwiftSyntax _version: 510.0.0)
...
#else
...
#endif

where the else block would still work for users of Swift 5.9 who by definition won't be able to use the swift-syntax aligned with 5.10 and therefore couldn't even reach the if anyway.

It does come with a certain danger of course, because presumably the package would have a more open ended dependency and it would rely on clients to constraint their dependencies in some way.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

also should we add a test?

We should, but it doesn't look like the infrastructure of the build plan tests supports it today. The version of any dependency seems to not be set when running those tests.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

Ah, it seems like we can just hardcode a version in the Manifest.createLocalSourceControlManifest() call, but we don't really resolve anything, so we currently don't.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

We're currently discussing whether we should bring this into 5.9, so for now I set it to be included for any package with tools-version 5.9 or newer.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

[157/171] Compiling BuildTests BuildPlanTests.swift
/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/Tests/BuildTests/BuildPlanTests.swift:4917:62: error: 'contains' is only available in macOS 13.0 or newer
            XCTAssertTrue(try swiftTarget.compileArguments().contains(["-user-module-version", "1.0.0"]))

wat

This would allow conditionalizing client code using ``#if canImport(:_version:)`` so that different versions of a dependency can be supported within a single codebase.

Note: this is explicitly not passing the flag if pre-releases are in use to avoid any confusion in those cases.

rdar://107077287
@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 8, 2023

@swift-ci please smoke test windows

@neonichu neonichu merged commit 6f0a64b into main Aug 9, 2023
5 checks passed
@neonichu neonichu deleted the user-module-version branch August 9, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants