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

Synthesize SWIFTPM_MODULE_BUNDLE for targets with Objc sources #4902

Closed
wants to merge 9 commits into from
Closed

Synthesize SWIFTPM_MODULE_BUNDLE for targets with Objc sources #4902

wants to merge 9 commits into from

Conversation

plu
Copy link

@plu plu commented Dec 2, 2022

Resolves #3785

Short description 📝

Synthesizes two more files into Derived/Sources, next to the already existing TuistBundle+TargetName.swift:

  • TuistBundle+TargetName.h
  • TuistBundle+TargetName.m

Only if the target contains Objective-C header files. The implementation is trying to mimic what SPM does.

How to test the changes locally 🧐

You can try to use the attached example app from #3785.

Checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase
  • The title of the PR will be used as changelog entry, please make sure it is clear and suitable
  • In case the PR introduces changes that affect users, the documentation has been updated
  • Contributors have checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed

Comment on lines +81 to +84
// FIXME: Is there any better way than GCC_PREFIX_HEADER? And what if the target would have set this already, then we're breakings something here.
var settings = modifiedTarget.settings?.base ?? SettingsDictionary()
settings["GCC_PREFIX_HEADER"] = .string(headerFile.path.url.relativePath)
modifiedTarget.settings = modifiedTarget.settings?.with(base: settings)
Copy link
Author

@plu plu Dec 2, 2022

Choose a reason for hiding this comment

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

This needs to be discussed, there's hopefully a better way, shouldn't stay like this.

The SPM implementation does this: -include path/to/resource_bundle_accessor.h

See also:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be added to the target headers directly rather than via build settings?

@danieleformichelli danieleformichelli added the changelog:added PR will be listed in the Added section of CHANGELOG label Dec 24, 2022
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 contributing this @plu

If I understood and followed along the issue, this issue mostly relate to when using the Dependencies feature of Tuist to integrate Swift Packages.

The changes made here will apply to all projects (ones that leverage the dependencies feature and ones that don't!) which is most likely not the intention here. Additionally even when using the dependencies feature I suspect this only needs to be applied to the external projects (the Tuist managed swift package projects).

Some suggestions:

  • Create a separate mapper which contains this logic and only applies for projects / targets managed by the Dependencies feature
  • Potentially omit the current mapper which generates Swift bundle accessors for projects / targets managed by the Dependencies feature as those won't be using those files

@danyf90 is something like this possible? Is there a way to distinguish between projects to control which mappers run?

Comment on lines +81 to +84
// FIXME: Is there any better way than GCC_PREFIX_HEADER? And what if the target would have set this already, then we're breakings something here.
var settings = modifiedTarget.settings?.base ?? SettingsDictionary()
settings["GCC_PREFIX_HEADER"] = .string(headerFile.path.url.relativePath)
modifiedTarget.settings = modifiedTarget.settings?.with(base: settings)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be added to the target headers directly rather than via build settings?

@danieleformichelli
Copy link
Collaborator

@danyf90 is something like this possible? Is there a way to distinguish between projects to control which mappers run?

I think it's possible (there should be an isExternal or something flag on project, but given we are creating it for all Swift target right now, why shouldn't we do the same for all ObjC targets?

@kwridan
Copy link
Collaborator

kwridan commented Jan 20, 2023

I think it's possible (there should be an isExternal or something flag on project,

Nice! found it https://github.com/tuist/tuist/blob/main/Sources/TuistGraph/Models/Project.swift#L60

but given we are creating it for all Swift target right now, why shouldn't we do the same for all ObjC targets?

Valid question :)

As things stand, ObjC can still leverage the generated bundle as I believe it has an @objc annotated interface. There could be an argument to making an ObjC accessor instead of the Swift one for targets that are ObjC only to avoid needing mixed language support.

For the above, the generated files would need to have a stand alone implementation to finding the bundle as opposed to referencing the Swift implementation and wouldn't need to have SWIFTPM_MODULE_BUNDLE defined as that only applicable to Swift Packages.

@github-actions
Copy link
Contributor

This PR has been marked as stale because it's been opened for more than 30 days. If no action is taken, it'll be closed in 5 days.

@danieleformichelli
Copy link
Collaborator

For the above, the generated files would need to have a stand alone implementation to finding the bundle as opposed to referencing the Swift implementation and wouldn't need to have SWIFTPM_MODULE_BUNDLE defined as that only applicable to Swift Packages.

@kwridan From the linked issue, it looks like there is some OBJC code in a dependency that tries to access it, so I guess SPM generates it and we need to do the same (maybe only for SPM generated targets?), isn't it?

@danieleformichelli
Copy link
Collaborator

Closing for inactivity, if you want to continue feel free to reopen it

@plu
Copy link
Author

plu commented Feb 27, 2023

Sorry for not responding, I have completely missed the notification for the comments on this PR :(.

Unfortunately our internal priorities have shifted, and currently it does not look like we're going for Tuist anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added PR will be listed in the Added section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dependencies.swift][SPM] Use of undeclared identifier 'SWIFTPM_MODULE_BUNDLE'
3 participants