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 Tool Plugin Support #733

Conversation

technocidal
Copy link

@technocidal technocidal commented Nov 19, 2022

Short description πŸ“

I'm an avid user of XcodeGen and would like to have Build Tool Plugin support. It's possible to implement this without having to change this project but it leads to temporary identifiers sticking around in the generated project.

More details can be found in the issue where I documented my research.

Solution πŸ“¦

Adds a flag to XCSwiftPackageProductDependency to indicate that it should be treated as a build tool plugin.

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

  • Add flag to XCSwiftPackageProductDependency
  • Prefix the product name with "plugin:" as this indicates to Xcode that this should be treated as a plugin
  • Check if a targets dependencies includes a package product that sets the flag to true
  • Fix the package product reference before continuing

…d be treated as a build tool plugin

Signed-off-by: Johannes Ebeling <14994778+technocidal@users.noreply.github.com>
Signed-off-by: Johannes Ebeling <14994778+technocidal@users.noreply.github.com>
Signed-off-by: Johannes Ebeling <14994778+technocidal@users.noreply.github.com>
@technocidal
Copy link
Author

@marciniwanicki @adamkhazi Just a quick ping: Is there anything I can do to move this along? Do you've any questions or suggestions regarding this feature? I'd be very grateful for any feedback.

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 @technocidal and for the investigation in yonaskolb/XcodeGen#1290 πŸ‘

It would be helpful if you're able to add a new fixture with the pbxproj file that contains a build plugin and have a a few tests read & write to allow verifying the serialisation and deserialisation works as expected.

Some examples of past fixtures https://github.com/tuist/XcodeProj/tree/main/Fixtures and associated tests https://github.com/tuist/XcodeProj/blob/main/Tests/XcodeProjTests/Scheme/XCSchemeTests.swift#L695

package: XCRemoteSwiftPackageReference? = nil) {
self.productName = productName
package: XCRemoteSwiftPackageReference? = nil,
plugin: Bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

to make this change non source breaking, can I trouble you to default the plugin parameter value?

Suggested change
plugin: Bool) {
plugin: Bool = false) {

Comment on lines +44 to +48
if let plugin = try container.decodeIfPresent(Bool.self, forKey: .plugin) {
self.plugin = plugin
} else {
plugin = false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if I followed along, there isn't a plugin attribute in the pbxproj file, rather the product name is prefixed with plugin: - in that case would this not require checking for that instead?

Copy link
Author

Choose a reason for hiding this comment

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

@kwridan Good point! I just realized that I completely misunderstood what this initializer is for. My bad πŸ€¦β€β™‚οΈ

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries

@kwridan
Copy link
Collaborator

kwridan commented Dec 6, 2022

@all-contributors add @technocidal for code

@allcontributors
Copy link
Contributor

@kwridan

I've put up a pull request to add @technocidal! πŸŽ‰

@iammxrn
Copy link

iammxrn commented Dec 20, 2022

Hey there! Any updates?

@clausjoergensen
Copy link

Hey, anything we can do to help speed this along? Getting the plugin support done (and integrated in Xcodegen) would be a massive benefit to my team, and we'd be happy to contribute.

Thanks!

@kwridan kwridan marked this pull request as draft January 20, 2023 18:37
@kwridan
Copy link
Collaborator

kwridan commented Jan 20, 2023

I've converted back draft while this is being worked on

@technocidal
Copy link
Author

technocidal commented Jan 20, 2023

I think I dropped the ball on this one. Sorry. I'll try to summarise my thoughts which I should've done way earlier.

I can totally understand that getting this done would be a great thing but I'm a bit worried that this will generate a lot of follow-up maintenance. I've also worked on the plugin support for SwiftLint which has turned into a massive headache for everyone involved. There are a lot of edge cases related to plugins that are not correctly handled by Xcode. This means that it is entirely possible that adding a plugin to your project breaks it. This happens with SwiftLint and it's dependency on swift-syntax quite a lot. You can find more details in this discussion.

There are also some other weird linker issues that we've filed a Feedback (FB11877146) for but haven't heard from Apple yet.

Regarding the actual implementation. I think I only need to update my fork and finish implementing the test suite/fixtures but I'm a bit disheartened due to the problems mentioned above.

@kwridan
Copy link
Collaborator

kwridan commented Jan 20, 2023

No worries at all and no rush.

Thank you for sharing some of gotchas with plugins - sorry to hear about the hurdles you had to face with it.

@clausjoergensen
Copy link

@technocidal any updates from Apple?

@github-actions
Copy link

Hola πŸ‘‹,

We want to let you know that your pull request has been marked as stale. It seems that there hasn't been any activity or updates on it for a while.

If you're still interested in having this pull request merged or reviewed, please provide any necessary updates or address any feedback that may have been given. We would be happy to continue the review process and consider merging it into the main branch.

However, if this pull request is no longer a priority or if you've decided to take a different approach, please let us know so we can close it accordingly.

Thank you for your understanding and contribution.

@github-actions github-actions bot added the stale label Jun 27, 2023
@github-actions
Copy link

github-actions bot commented Jul 2, 2023

Hola πŸ‘‹,

We want to let you know that we have decided to close your pull request #456 due to prolonged inactivity. Despite the initial interest and efforts put into the pull request, we haven't seen any updates or responses for a considerable period of time.

We understand that circumstances change and priorities shift, which may have led to this inactivity. If you still wish to contribute or have further discussions on this feature or bug fix, please don't hesitate to reopen the pull request and engage with the community.

We appreciate your understanding and the time you invested in submitting the pull request. Your contributions are valuable, and we hope to collaborate with you on future endeavors.

Thank you.

@github-actions github-actions bot closed this Jul 2, 2023
kwridan pushed a commit that referenced this pull request Jul 27, 2023
- Updated `XCSwiftPackageProductDependency` to support parsing and creating build tool plugins

Notes:

- This is a continuation of #733, to support **Build Tool Plug-ins**. (Thanks to @technocidal)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants