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

Use StaticInternalSwiftSyntaxParser #1037

Conversation

liamnichols
Copy link
Collaborator

Background

Sourcery depends on lib_internalSwiftSyntaxParser to use SwiftSyntax, but since this is deployed as a dynamic library by default, it becomes problematic when it comes to distributing the sourcery binary.

I happened to notice last week that SwiftLint recently released 0.47.0-rc.1, that also depends on SwiftSyntax and I happened to notice that they're using a static library version of lib_internalSwiftSyntaxParser - https://github.com/keith/StaticInternalSwiftSyntaxParser.

We're currently evaluating integrating Sourcery into our project, and we currently use Mint as our primary way of integrating Swift tooling. I believe its important for us to be able to lock the versions of the tools that we use for consistency, which is why I'm hesitant to use Homebrew, and I'd ideally want to avoid using another system that we're not already using so instead I thought it might be worth seeing if its possible to leverage StaticInternalSwiftSyntaxParser so that using Mint doesn't have any downsides 🙂

Changes

While I'll mark this PR as ready for review, I don't think we're quite there yet but I want to use what I have so far to get a conversation going. I'll leave more comments inline about specifics, but other than that, I understand that Sourcery is quite a big ecosystem (SourceryFramework, distributed by CocoaPods etc) so I'm sure there is also something else I have probably missed 😄

Thanks again for such a cool tool though!

Copy link
Collaborator Author

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Adding some notes based on my observations as I implemented the change 🙂

Package.swift Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
Rakefile Show resolved Hide resolved
Rakefile Show resolved Hide resolved
@liamnichols liamnichols marked this pull request as ready for review March 15, 2022 18:30
Rakefile Outdated Show resolved Hide resolved
@AF-cgi
Copy link

AF-cgi commented Mar 18, 2022

I think the best solution is to use the following release of swift-syntax: https://github.com/apple/swift-syntax/releases/tag/0.50600.1.

@AF-cgi AF-cgi mentioned this pull request Mar 18, 2022
@liamnichols
Copy link
Collaborator Author

Following some discussion in #1038, there are some other things to think about related to this change.

@krzysztofzablocki
Copy link
Owner

@liamnichols this is such a great work 🙇

I don't think we need backward compatibility, we can bump SwiftSyntax version if it doesn't require api changes (most of the time it doesn't)

@AF-cgi
Copy link

AF-cgi commented Mar 21, 2022

I would prefer to use the correct version of SwiftSyntax. But I'm not 100% if the local toolchain is the problem. I use Sourcery via Mint. That means: if you work with Xcode 13.2.1 and use Sourcery via Mint, then you will build a version of Sourcery in Swift 5.5.2. If you do the same thing with Xcode 13.3, then you will build it on Swift 5.6. I think in both cases you need a different version of SwiftSyntax. That means we need a flexible solution to add the correct version of SwiftSyntax.

@krzysztofzablocki
Copy link
Owner

@AF-cgi this only happens because Mint is not supported, rPath should be pointing to the library I bundled with the tool otherwise you are going to crash sooner or later.

The changes @liamnichols did here mean we are no longer dynamically linking but bundling the library as any other dependency, which will mean it will work regardless of what Xcode version you use even in Mint config

Copy link
Collaborator Author

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Thanks @krzysztofzablocki, I'll take a look at getting this ready for Xcode 13.3/Swift 5.6 then and let you know how I get on

Rakefile Outdated Show resolved Hide resolved
Rakefile Show resolved Hide resolved
@Cyberbeni
Copy link
Contributor

Cyberbeni commented Mar 22, 2022

When using swift build --package-path XXX --configuration release with Xcode 13.2.1, I get the following warning:
ld: warning: dylib (/Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/lib_InternalSwiftSyntaxParser.dylib) was built for newer macOS version (10.14.6) than being linked (10.12)

https://github.com/Cyberbeni/install-swift-tool/runs/5650524281?check_suite_focus=true#step:4:177

edit: I assume it is trying to link the dylib from Xcode because the binaryTarget specification requires Swift 5.6 to work properly (Updating the swift-syntax package to 0.50600.1 should allow building with Xcode 13.2.1). Still should update platforms to .macOS("10.14.6") or higher in Package.swift.

edit2: // swift-tools-version:5.3 should also be updated to 5.5 (Xcode 13.0 is required to build even without the linking issues) -- or 5.6 if updating swift-syntax package doesn't allow linking correctly with Xcode 13.0-13.2.1

@liamnichols
Copy link
Collaborator Author

When using swift build --package-path XXX --configuration release with Xcode 13.2.1, I get the following warning: ld: warning: dylib (/Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/lib_InternalSwiftSyntaxParser.dylib) was built for newer macOS version (10.14.6) than being linked (10.12)

https://github.com/Cyberbeni/install-swift-tool/runs/5650524281?check_suite_focus=true#step:4:177
edit: I assume it is trying to link the dylib from Xcode because the binaryTarget specification requires Swift 5.6 to work properly (Updating the swift-syntax package to 0.50600.1 should allow building with Xcode 13.2.1). Still should update platforms to .macOS("10.14.6") or higher in Package.swift.

Ah yes, I had seen this too I think. I agree though, we should update Package.swift to match the minimum deployment target of lib_InternalSwiftSyntaxParser since I assume it wouldn't work on macOS 10.12 anyway (although I haven't/can't test).

For reference, the dlyb in Xcode 13.2.1:

$ otool -l /Applications/Xcode-13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/lib_InternalSwiftSyntaxParser.dylib | grep minos -B 4 -A 4
Load command 8
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform 1
    minos 10.14.6
      sdk 12.1
   ntools 1
     tool 3
  version 711.0
--
Load command 9
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform 1
    minos 11.0
      sdk 12.1
   ntools 1
     tool 3
  version 711.0

And in 13.3:

$ otool -l /Applications/Xcode-13.3.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx/lib_InternalSwiftSyntaxParser.dylib | grep minos -B 4 -A 4
Load command 8
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform 1
    minos 10.14.6
      sdk 12.3
   ntools 1
     tool 3
  version 762.0
--
Load command 9
      cmd LC_BUILD_VERSION
  cmdsize 32
 platform 1
    minos 11.0
      sdk 12.3
   ntools 1
     tool 3
  version 762.0

The minos remains 10.14.6 (or 11.0 for arm64).

I can update this in the Package.swift, but I guess its only something minor.

edit2: // swift-tools-version:5.3 should also be updated to 5.5 (Xcode 13.0 is required to build even without the linking issues) -- or 5.6 if updating swift-syntax package doesn't allow linking correctly with Xcode 13.0-13.2.1

Good shout 👍

@liamnichols liamnichols force-pushed the ln/static-lib_internalSwiftSyntaxParser branch from bddbeb5 to 659b388 Compare March 23, 2022 11:32
Copy link
Collaborator Author

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Making progress, but ran into a couple of test failures after updating SwiftSyntax:

SourceryLibTests.SwiftTemplateTests
SwiftTemplate__generates_correct_output, failed - expected to not throw any error, got <Building for debugging...[1/34] Emitting module SourceryRuntime[2/44] Compiling SourceryRuntime AccessLevel.swift[3/44] Compiling SourceryRuntime Annotations.swift[4/44] Compiling SourceryRuntime Array+Parallel.swift[5/44] Compiling SourceryRuntime Array.swift[6/44] Compiling SourceryRuntime AssociatedType.swift[7/44] Compiling SourceryRuntime Attribute.swift[8/44] Compiling SourceryRuntime AutoHashable.generated.swift[9/44] Compiling SourceryRuntime BytesRange.swift[10/44] Compiling SourceryRuntime Class.swift[11/44] Compiling SourceryRuntime Closure.swift[12/44] Compiling SourceryRuntime Coding.generated.swift[13/44] Compiling SourceryRuntime Struct.swift[14/44] Compiling SourceryRuntime Subscript.swift[15/44] Compiling SourceryRuntime TemplateContext.swift[16/44] Compiling SourceryRuntime Tuple.swift[17/44] Compiling SourceryRuntime Type.swift[18/44] Compiling SourceryRuntime TypeName.swift[19/44] Compiling SourceryRuntime Typealias.swift[20/44] Compiling SourceryRuntime Typed.generated.swift[21/44] Compiling SourceryRuntime Typed.swift[22/44] Compiling SourceryRuntime Variable.swift[23/44] Compiling SourceryRuntime Generic.swift[24/44] Compiling SourceryRuntime GenericRequirement.swift[25/44] Compiling SourceryRuntime Import.swift[26/44] Compiling SourceryRuntime JSExport.generated.swift[27/44] Compiling SourceryRuntime Log.swift[28/44] Compiling SourceryRuntime Method.swift[29/44] Compiling SourceryRuntime Modifier.swift[30/44] Compiling SourceryRuntime PhantomProtocols.swift[31/44] Compiling SourceryRuntime Protocol.swift[32/44] Compiling SourceryRuntime ProtocolComposition.swift[33/44] Compiling SourceryRuntime Composer.swift[34/44] Compiling SourceryRuntime Definition.swift[35/44] Compiling SourceryRuntime Description.generated.swift[36/44] Compiling SourceryRuntime Dictionary.swift[37/44] Compiling SourceryRuntime Diffable.generated.swift[38/44] Compiling SourceryRuntime Diffable.swift[39/44] Compiling SourceryRuntime Documentation.swift[40/44] Compiling SourceryRuntime Enum.swift[41/44] Compiling SourceryRuntime Equality.generated.swift[42/44] Compiling SourceryRuntime Extensions.swift[43/44] Compiling SourceryRuntime FileParserResult.swift[44/46] Emitting module SwiftTemplate[45/46] Compiling SwiftTemplate main.swift[46/46] Linking SwiftTemplateBuild complete! (19.73s)2022-03-23 11:52:06.902 xcodebuild[27043:55376] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionSentinelHostApplications for extension Xcode.DebuggerFoundation.AppExtensionHosts.watchOS of plug-in com.apple.dt.IDEWatchSupportCore2022-03-23 11:52:06.902 xcodebuild[27043:55376] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionPointIdentifierToBundleIdentifier for extension Xcode.DebuggerFoundation.AppExtensionToBundleIdentifierMap.watchOS of plug-in com.apple.dt.IDEWatchSupportCore2022-03-23 11:52:07.912 xcodebuild[27044:55384] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionSentinelHostApplications for extension Xcode.DebuggerFoundation.AppExtensionHosts.watchOS of plug-in com.apple.dt.IDEWatchSupportCore2022-03-23 11:52:07.912 xcodebuild[27044:55384] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionPointIdentifierToBundleIdentifier for extension Xcode.DebuggerFoundation.AppExtensionToBundleIdentifierMap.watchOS of plug-in com.apple.dt.IDEWatchSupportCore>
/Users/distiller/project/Tests/SourceryLibTests/Generating/SwiftTemplateSpecs.swift:65

          it("generates correct output") {
              expect { try Sourcery(cacheDisabled: true).processFiles(.sources(Paths(include: [Stubs.sourceDirectory])), usingTemplates: Paths(include: [templatePath]), output: output) }.toNot(throwError())

SwiftTemplate__generates_correct_output, failed - expected to equal <// Generated using Sourcery Major.Minor.Patch — https://github.com/krzysztofzablocki/Sourcery// DO NOT EDITextension Bar: Equatable {}// Bar has Annotationsfunc == (lhs: Bar, rhs: Bar) -> Bool {if lhs.parent != rhs.parent { return false }if lhs.otherVariable != rhs.otherVariable { return false }return true}extension Foo: Equatable {}func == (lhs: Foo, rhs: Foo) -> Bool {if lhs.name != rhs.name { return false }if lhs.value != rhs.value { return false }return true}extension FooSubclass: Equatable {}func == (lhs: FooSubclass, rhs: FooSubclass) -> Bool {if lhs.other != rhs.other { return false }return true}>, got (use beNil() to match nils)
/Users/distiller/project/Tests/SourceryLibTests/Generating/SwiftTemplateSpecs.swift:68

              let result = (try? (outputDir + Sourcery().generatedPath(for: templatePath)).read(.utf8))
              expect(result).to(equal(expectedResult))
          }

Executed 572 tests, with 2 failures (0 unexpected) in 114.289 (114.574) seconds

Will take a look, plus cover a few other things I spotted.

.circleci/config.yml Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Rakefile Show resolved Hide resolved
Package.swift Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Rakefile Show resolved Hide resolved
@liamnichols liamnichols force-pushed the ln/static-lib_internalSwiftSyntaxParser branch from a06cc8d to 21df065 Compare March 23, 2022 13:39
@liamnichols liamnichols force-pushed the ln/static-lib_internalSwiftSyntaxParser branch from 8c5dc81 to 670e799 Compare March 23, 2022 14:52
Copy link
Collaborator Author

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Added some more notes, I think this is good to go though.

One thing that I'm not sure about though is how we remove this line: https://github.com/Homebrew/homebrew-core/blob/ab5040f62247ae337489653254bd817f33645006/Formula/sourcery.rb#L23

SourcerySwift/Sources/SwiftTemplate.swift Show resolved Hide resolved
Rakefile Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Owner

@krzysztofzablocki krzysztofzablocki left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing it, so much easier to manager :)

@liamnichols
Copy link
Collaborator Author

Thanks for merging @krzysztofzablocki! One thing that I think I forgot to mention is that when you next to release to Homebrew, I think we'll need to remove this line manually:

https://github.com/Homebrew/homebrew-core/blob/71ed492e4ed7e55ebc5a77af8794067b5f21f037/Formula/sourcery.rb#L23

Other than that, it should be all good 👍

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