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

[Build/PackageGraph] Switch BuildSubset.{product, target} and `Modu… #7650

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jun 10, 2024

…lesGraph.{product, target}(...)` to use optional destination

This is mostly an NFC change. It makes more sense to default destination to "undefined" when applicable because it's not always possible to know intended destination based on user input.

Motivation:

This kind of design makes more sense for internal APIs, instead of having users to pass .destination even though it might not be always correct.

Modifications:

  • BuildSubset.{product, target} and ModulesGraph.{product, target}(...) accept an optional destination that is defaulted to .none

Result:

Easier to understand and use APIs to request a build subset and product and module references from the modules graph.

@xedin xedin added build system Changes to interactions with build systems modules graph Modules dependency resolution labels Jun 10, 2024
@xedin
Copy link
Contributor Author

xedin commented Jun 10, 2024

@swift-ci please test

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 modulo if expression formatting

…lesGraph.{product, target}(...)` to use optional destination

This is mostly an NFC change. It makes more sense to default `destination`
to "undefined" when applicable because it's not always possible to know
intended destination based on user input.
@xedin xedin force-pushed the make-buildset-optional-destination branch from 49ce2e5 to 3f4d7ef Compare June 10, 2024 21:11
@xedin
Copy link
Contributor Author

xedin commented Jun 10, 2024

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Jun 10, 2024

@swift-ci please test Windows platform

@xedin xedin merged commit d98b753 into swiftlang:main Jun 11, 2024
5 checks passed
xedin added a commit that referenced this pull request Jun 14, 2024
…`Modu… (#7668)

…lesGraph.{product, target}(...)` to use optional destination

- Explanation:

  Follow-up to #7646
  
This is mostly NFC change. It makes more sense to default `destination`
to "undefined" when applicable because it's not always possible to know
intended destination based on user input.

This kind of design makes more sense for internal APIs, instead of
having users to pass `.destination` even though it might not be always
correct.

- Main Branch PRs:
#7650

- Risk: Very Low

- Reviewed By: @MaxDesiatov 

- Testing: Existing test-cases were modified and new tests were added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system Changes to interactions with build systems modules graph Modules dependency resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants