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 nested umbrella header handling #3588

Merged
merged 5 commits into from
Oct 26, 2021

Conversation

danieleformichelli
Copy link
Collaborator

Resolves #3459

Short description πŸ“

Nested umbrella headers should be handled as umbrella headers (adding the headers to the build phase instead of using the module map)

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.
  • In case the PR introduces changes that affect how the cache artifact is generated starting from the TuistGraph.Target, the Constants.cacheVersion has been updated.

@pepicrft
Copy link
Contributor

It'd be great to get @kwridan's eyes on this one

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

If I followed along the nestedHeader is the case is what GoogleSignIn has

e.g.

GoogleSignIn/Sources/
β”œβ”€β”€ .....
β”œβ”€β”€ Public
β”‚   └── GoogleSignIn
β”‚       β”œβ”€β”€ GIDAuthentication.h
β”‚       β”œβ”€β”€ GIDConfiguration.h
β”‚       β”œβ”€β”€ GIDGoogleUser.h
β”‚       β”œβ”€β”€ GIDProfileData.h
β”‚       β”œβ”€β”€ GIDSignIn.h
β”‚       β”œβ”€β”€ GIDSignInButton.h
β”‚       └── GoogleSignIn.h

And GoogleSignIn.h is the umbrella header. From that perspective the changes make sense - to expose those as public headers instead of using a custom module map.

Inspecting the GoogleSignIn project in isolation I see some of the other headers are missing - is that expected?

e.g.

Original (stand alone generated using swift package generate-xcodeproj

original

vs. tuist generate

generated

Co-authored-by: Kas <kwridan@users.noreply.github.com>
@danieleformichelli
Copy link
Collaborator Author

Hey @kwridan πŸ‘‹
Thanks for the review!

Regarding the missing headers, the public ones are only the ones we have added, while other ones are referenced locally via the header search path.
We could add them to the project as additional files to be shown in Xcode, but I think it is outside of the scope of this PR

@fortmarek fortmarek merged commit 4000af0 into main Oct 26, 2021
@fortmarek fortmarek deleted the fix/nested-umbrella-header branch October 26, 2021 11:25
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.

πŸ“¦ [Dependencies.swift][SPM] Fix GoogleSignIn dependency
4 participants