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

fix: add missing BuildSettingsProvider for visionOS #898

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

alexanderwe
Copy link
Collaborator

Short description 📝

Tuist introduced XcodeGraph at version 0.19.4 with this PR tuist/tuist#7083. This in fact caused this PR to take effect tuist/XcodeGraph#79. This then ultimately caused this codepath https://github.com/tuist/tuist/blob/main/Sources/TuistGenerator/Settings/DefaultSettingsProvider.swift#L115 to not include visionOS related default settings.

Solution 📦

I added the mapping of visionOS to the BuildSettingsProvider in order to also correctly set the settings for visionOS.

Implementation 👩‍💻👨‍💻

  • Add visionOS mappings to BuildSettingsProvider
  • Update BuildSettingProviderTests to test the newly added mappings

Questions

  • @fortmarek / @pepicrft I was not quite sure if we should also set CODE_SIGN_IDENTITY on the visionOS platform in order to be aligned with the iOS one - what do you think ?
  • Please let me know if the explanation is good enough or if I missed something or have invalid reasoning.

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 @alexanderwe

Sources/XcodeProj/Utils/BuildSettingsProvider.swift Outdated Show resolved Hide resolved
@pepicrft
Copy link
Contributor

@fortmarek / @pepicrft I was not quite sure if we should also set CODE_SIGN_IDENTITY on the visionOS platform in order to be aligned with the iOS one - what do you think ?

I think that makes sense. Thoughts @fortmarek ?

@fortmarek
Copy link
Member

I think that makes sense. Thoughts @fortmarek ?

I'm aligned with that as well 👍

@pepicrft
Copy link
Contributor

In that case let's propagate your changes as suggested by @kwridan and we can then merge the PR. Thanks a lot @alexanderwe for this one 🚀

Add new test cases for additional visionOS mappings
Add tests for missing watchOS cases
@alexanderwe
Copy link
Collaborator Author

@kwridan @pepicrft @fortmarek
Thanks a lot for your reviews !

I have rechecked the cases and added visionOS where needed. In addition I have now also added unit tests for these new cases. While doing that I also saw that the BuildSettingProviderTests was also missing tests for watchOS. I have added these tests as well now - if you think these should rather be in another PR to keep this one more atomic just let me know.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good 🙂

@fortmarek
Copy link
Member

While doing that I also saw that the BuildSettingProviderTests was also missing tests for watchOS. I have added these tests as well now - if you think these should rather be in another PR to keep this one more atomic just let me know.

These minor fly-by changes are fine – thanks for improving that!

@fortmarek fortmarek changed the title fix: Fix BuildSettingsProvider for visionOS fix: fix BuildSettingsProvider for visionOS Jan 24, 2025
@fortmarek fortmarek changed the title fix: fix BuildSettingsProvider for visionOS fix: add missing BuildSettingsProvider for visionOS Jan 24, 2025
@fortmarek fortmarek merged commit efdc4f5 into main Jan 24, 2025
10 of 11 checks passed
@fortmarek fortmarek deleted the feature/fix-build-settings-provider-for-visionos branch January 24, 2025 18:27
@alexanderwe
Copy link
Collaborator Author

@fortmarek Thanks for merging !

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.

4 participants