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

Make it impossible to create an invalid BuildSettingCondition #4131

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Make it impossible to create an invalid BuildSettingCondition #4131

merged 1 commit into from
Feb 15, 2022

Conversation

sjavora
Copy link
Contributor

@sjavora sjavora commented Feb 12, 2022

Instead of a single when method that takes two optional values, use multiple methods so that it is impossible for both to be nil.

tomerd
tomerd reviewed Feb 12, 2022
View reviewed changes
Sources/PackageDescription/BuildSettings.swift Outdated
///
/// - Parameters:
/// - platforms: The applicable platforms for this build setting condition.
public static func when(platforms: [Platform]) -> BuildSettingCondition {
BuildSettingCondition(platforms: platforms, config: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use .none instead of nil to make it more semantically clear

tomerd
tomerd reviewed Feb 12, 2022
View reviewed changes
Sources/PackageDescription/BuildSettings.swift Outdated
precondition(!(platforms == nil && configuration == nil))
return BuildSettingCondition(platforms: platforms, config: configuration)
public static func when(configuration: BuildConfiguration) -> BuildSettingCondition {
BuildSettingCondition(platforms: nil, config: configuration)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use .none instead of nil to make it more semantically clear

@tomerd
Copy link
Contributor

tomerd commented Feb 12, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Feb 12, 2022

thanks @sjavora

@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from 23756a4 to fbe0d7a Compare February 12, 2022 20:09
@sjavora
Copy link
Contributor Author

sjavora commented Feb 12, 2022

@tomerd Updated to use .none 👍

In the meantime I also found https://github.com/apple/swift-package-manager/blob/2ea136b5703039b5a596c8a61d090e4dc300ee98/Sources/PackageDescription/PackageDescriptionSerialization.swift#L354

...which seems like it got copy/pasted as there is just one optional parameter. Should I remove that as well? TargetDependencyCondition is Encodable so I'm not 100% sure removing the optionality won't break something...

@tomerd
Copy link
Contributor

tomerd commented Feb 12, 2022

thanks @sjavora

PackageDescriptionSerialization is a bit more sensitive since it drives the manifest parsing. this does need to be improved, but maybe a topic for a separate PR

@tomerd
Copy link
Contributor

tomerd commented Feb 12, 2022

@swift-ci please smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Feb 12, 2022
@SDGGiesbrecht
Copy link
Contributor

This is a public, source‐breaking change to the PackageDescription module. (For example, .when(platforms: [.macOS], configuration: nil) will break.) Does the old method need to be retained with tools version constraints and/or deprecation notices?

@tomerd
Copy link
Contributor

tomerd commented Feb 13, 2022 •
edited
Loading

This is a public, source‐breaking change to the PackageDescription module. (For example, .when(platforms: [.macOS], configuration: nil) will break.) Does the old method need to be retained with tools version constraints and/or deprecation notices?

good catch @SDGGiesbrecht did not notice this was in PackageDescription. @sjavora please lets restore the previous API but mark as deprecated, and add availability notation on the new APIs as well e.g..

@available(_PackageDescription, deprecated: 999.0)
public static func when(
         platforms: [Platform]? = nil,
         configuration: BuildConfiguration? = nil
     ) -> BuildSettingCondition

@available(_PackageDescription, introduced: 999.0)
public static func when(
         platforms: [Platform]
     ) -> BuildSettingCondition

we will update 999.0 to 5.7 when we add that version identifier

@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from fbe0d7a to a2bc957 Compare February 13, 2022 10:11
@sjavora
Make it impossible to create an invalid BuildSettingCondition.
60f0253 
Instead of a single method that takes two optional values, use multiple methods so that it is impossible for both to be `nil`.
@sjavora sjavora force-pushed the disallowImpossibleBuildSettingCondition branch from a2bc957 to 60f0253 Compare February 13, 2022 10:13
@sjavora
Copy link
Contributor Author

sjavora commented Feb 13, 2022

Oops! I didn't even think about that 🤦

Thank you for noticing - put back the original method and added @availability.

@tomerd
Copy link
Contributor

tomerd commented Feb 14, 2022 •
edited
Loading

thanks @sjavora this looks good now imo. @SDGGiesbrecht wdyt?

one last ask, since this is user facing API change in the manifest could you please add an entry in the change log, e.g. #4137 (can be done in a separate PR if you prefer)

@SDGGiesbrecht
Copy link
Contributor

👍 Looks good now.

@tomerd
Copy link
Contributor

tomerd commented Feb 14, 2022

@swift-ci please smoke test

@shahmishal
Copy link
Member

@swift-ci please smoke test

4 similar comments
@tomerd
Copy link
Contributor

tomerd commented Feb 14, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Feb 14, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Feb 14, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Feb 15, 2022

@swift-ci please smoke test

@shahmishal
Copy link
Member

@swift-ci please smoke test

1 similar comment
@tomerd
Copy link
Contributor

tomerd commented Feb 15, 2022

@swift-ci please smoke test

@tomerd tomerd merged commit 645fe16 into swiftlang:main Feb 15, 2022
tomerd added a commit that referenced this pull request Feb 15, 2022
@tomerd
Update CHANGELOG.md
a57768e 
motivation: keep change log up to date

changes: document changes from #4131
@tomerd tomerd mentioned this pull request Feb 15, 2022
Merged
@sjavora sjavora deleted the disallowImpossibleBuildSettingCondition branch February 15, 2022 08:11
tomerd added a commit that referenced this pull request Feb 15, 2022
@tomerd
Update CHANGELOG.md (#4141)
a06b331 
motivation: keep change log up to date

changes: document changes from #4131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@neonichu neonichu Awaiting requested review from neonichu

@elsh elsh Awaiting requested review from elsh

Assignees
No one assigned
Labels
add to change log ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sjavora @tomerd @SDGGiesbrecht @shahmishal