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

Using StaticInternalSwiftSyntaxParser? #473

Closed
liamnichols opened this issue Mar 29, 2022 · 5 comments
Closed

Using StaticInternalSwiftSyntaxParser? #473

liamnichols opened this issue Mar 29, 2022 · 5 comments

Comments

@liamnichols
Copy link
Collaborator

liamnichols commented Mar 29, 2022

👋

I've recently worked on krzysztofzablocki/Sourcery#1037, which took inspiration from realm/SwiftLint#3867 which essentially updates the tools to use StaticInternalSwiftSyntaxParser to make the binaries more portable by removing the dependency on a dylib.

Before taking on any work (since it can get a bit tricky), I wanted to check to see if you'd be keen in taking this direction with Periphery?

Benefits:

  • Portable binary
  • No blockers from building with newer toolchains without having to update SwiftSyntax dependency

Starting with SwiftSyntax for 5.6, the lib_internalSwiftSyntaxParser.dlyb dependency is now a binary dependency in the Package.swift file, which is better than relying on the internal version bundled in Xcode (and linked to that version of Xcode via the rpath), which is a great step in the right direction, but it still has the portability issues described above.

@liamnichols liamnichols changed the title Using StaticInternalSwiftSyntaxParser Using StaticInternalSwiftSyntaxParser? Mar 29, 2022
@liamnichols
Copy link
Collaborator Author

Needs a bit more work, but I'm experimenting here - liamnichols/periphery@master...ln/static-lib-internal-swift-syntax-parser

There are a couple of extra things to consider since I had to update the package tools version to 5.3 in order to reference .binaryTarget, and we'd also want to support Swift 5.6 (see also #471), but it works enough for me to test our tooling using Xcode 13.3 now

@ileitch
Copy link
Contributor

ileitch commented Apr 4, 2022

Hey @liamnichols, yes this sounds like a great idea. Thanks for working on this!

@liamnichols
Copy link
Collaborator Author

liamnichols commented Apr 5, 2022

No problem, although I think i've become a bit blocked by SR-15243 (and SR-15065 when working around things with a local binary target).

Running swift package describe (which RetentionTest requires) errors after introducing a binary target. I'll try to better reproduce the issues and report/update the bug ticket.

@liamnichols
Copy link
Collaborator Author

@ileitch: I was a bit on the fence with the approach that I wanted to take here, so now there are two 😄

I updated the descriptions with the info, please feel free to digest it whenever you get a chance and let me know what you think 😄 Maybe there is even a better third option hiding somewhere as well

@liamnichols
Copy link
Collaborator Author

Closing this now since #480 merged to master, thanks!

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

No branches or pull requests

2 participants