-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for traits in manifest code generation #9173
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
base: main
Are you sure you want to change the base?
Conversation
@swift-ci test |
@swift-ci test |
|
||
extension PackageDependency.Trait { | ||
fileprivate static func precedes(_ a: PackageDependency.Trait, _ b: PackageDependency.Trait) -> Bool { | ||
if a.name != b.name { return a.name < b.name } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I'm not sure we can ever have two traits of the same name in a single package (cc @FranzBusch is this a correct assumption?), so sorting traits that originate from the same package can probably just be done by using traits.sorted(by: { $0.name < $1.name })
(this can similarly be done for the traits listed in the condition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, if we unify the directives, sorting becomes simpler, which is nice, but...
At present, the following is valid:
dependencies: [
.package(url: "https://github.com/swiftlang/swift-syntax.git", from: "601.0.1", traits: [
.trait(name: "Foo", condition: .when(traits: ["Blue"])),
.trait(name: "Foo", condition: .when(traits: ["Green"])),
]),
],
It can be unified as follows:
dependencies: [
.package(url: "https://github.com/swiftlang/swift-syntax.git", from: "601.0.1", traits: [
.trait(name: "Foo", condition: .when(traits: ["Blue", "Green"])),
]),
],
However, for example, if it is extended like this...
dependencies: [
.package(url: "https://github.com/swiftlang/swift-syntax.git", from: "601.0.1", traits: [
.trait(name: "Foo", condition: .when(platforms: [.macOS], traits: ["Blue"])),
.trait(name: "Foo", condition: .when(platforms: [.linux], traits: ["Green"])),
]),
],
This does not seem to be unifiable.
In the future, if it ever comes to this, we might overlook the necessary adjustments.
What should we do about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two parts to traits.
- The place where they are defined
- The place where they are enabled for a given dependency
In the former, trait names can only be expressed once. In the latter, @omochi is right that it is totally valid to define an enabled trait multiple times with different conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omochi Thank you for the detailed examples! This helps a lot.
} | ||
|
||
// represents `.defaults` | ||
public var isDefaultsCase: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the lack of explanation.
The main purpose of this patch is not to make it public
.
However, since PackageModel.TraitDescription.isDefault
already exists and is public
, I thought it would be natural to make PackageModel.PackageDependency.Trait.isDefaultsCase
public as well.
In particular, for this part, the user-facing API uses .defaults
, while the internal representation uses "default"
, which do not match. This makes it easy to make mistakes during implementation, so I thought it would be helpful to provide an API.
|
||
extension PackageDependency.Trait { | ||
fileprivate static func precedes(_ a: PackageDependency.Trait, _ b: PackageDependency.Trait) -> Bool { | ||
if a.name != b.name { return a.name < b.name } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two parts to traits.
- The place where they are defined
- The place where they are enabled for a given dependency
In the former, trait names can only be expressed once. In the latter, @omochi is right that it is totally valid to define an enabled trait multiple times with different conditions.
@swift-ci please test windows |
Add support for traits in manifest code generation
Motivation:
Currently, manifest code generation does not support the trait feature. This results in incomplete or incorrect generated manifests whenever traits are involved. To ensure manifests accurately reflect the package definition, trait-related constructs need to be supported.
Modifications:
This patch implements trait support in manifest code generation. Specifically, it adds support for three syntactic forms:
Result:
Manifest code generation will correctly emit trait-related constructs. Packages that define traits, or that depend on other packages/targets conditionally based on traits, will now be represented accurately in the generated manifest.