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

Add buildArchitectures to XCScheme+BuildAction #824

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

woin2ee
Copy link
Contributor

@woin2ee woin2ee commented Jun 8, 2024

This PR is related to tuist/tuist#6003

Short description 📝

It makes enable to set Override Architectures option of build action.

Solution 📦

I observed the changes in the xcscheme file as I changed the Override Architectures option. After that, I decided to add the attribute in a similar way to the existing implementation of buildImplicitDependencies and runPostActionsOnFailure.

Implementation 👩‍💻👨‍💻

  • Add an enum to represent the Override Architectures option.
  • Add buildArchitectures property in XCScheme.BuildAction.
  • Add and Update tests

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

nice work @woin2ee - thanks for contributing this

case "All":
self = .universal
default:
self = .matchRunDestination
Copy link
Collaborator

Choose a reason for hiding this comment

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

From testing in a Xcode, unspecified or random values default to useTargetSettings

Suggested change
self = .matchRunDestination
self = .useTargetSettings

Alternatively can make this an failable init (init?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. It’s my mistake. 🙏

@@ -84,11 +116,13 @@ extension XCScheme {
postActions: [ExecutionAction] = [],
parallelizeBuild: Bool = false,
buildImplicitDependencies: Bool = false,
runPostActionsOnFailure: Bool? = nil) {
runPostActionsOnFailure: Bool? = nil,
buildArchitectures: Architectures = .matchRunDestination) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an interesting one

  • New schemes created in the Xcode UI do indeed default to .matchRunDestination
  • The current released version of XcodeProj doesn't explicitly write this attribute when creating new schemes (i.e. would be .useTargetSettings)

Having matchRunDestination as the new default for the init here would introduce a change in behaviour for existing clients of the library and all their generated schemes would go from .useTargetSettings > .matchRunDestination 🙈

Suggested change
buildArchitectures: Architectures = .matchRunDestination) {
buildArchitectures: Architectures = .useTargetSettings) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current released version didn't have control over this attribute, so I thought it wouldn't affect existing clients as if it were a new feature.
However, I missed the fact that there may be clients who are using the current version's settings well. Thank you for your review.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @woin2ee

@kwridan
Copy link
Collaborator

kwridan commented Jun 20, 2024

@all-contributors add @woin2ee for code

Copy link
Contributor

@kwridan

I've put up a pull request to add @woin2ee! 🎉

@kwridan kwridan requested a review from pepicrft July 5, 2024 06:57
@pepicrft pepicrft merged commit 7c94909 into tuist:main Jul 8, 2024
5 checks passed
@woin2ee woin2ee deleted the scheme-override-architectures branch July 8, 2024 13:22
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.

3 participants