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

Add a dynamic and static library product of SwiftSyntax to Package.swift #26

Merged
merged 1 commit into from
Nov 3, 2018

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Nov 3, 2018

Address #25 (comment).

We should not specify the type of the library product via an environment variable. Instead a cleaner approach is to have both a static and a dynamic library product in the package and just build the type of the library that is needed.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 3, 2018

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Nov 3, 2018

CC @nkcsgexi

@nkcsgexi nkcsgexi merged commit d2d16ed into swiftlang:master Nov 3, 2018
@nkcsgexi
Copy link
Contributor

nkcsgexi commented Nov 3, 2018

Thanks, @ahoppen ! I was just about to do this myself. Your comment makes total sense.

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Nov 3, 2018

Wait, i get this error when building locally:

Linking ./.build/x86_64-apple-macosx10.10/debug/libSwiftSyntax-dynamic.dylib
Compile Swift Module 'lit_test_helper' (3 sources)
<unknown>:0: error: module name "SwiftSyntax-dynamic" is not a valid identifier

We don't wanna change either the dylib name or the module name. Any idea how to resolve?

@ahoppen
Copy link
Member Author

ahoppen commented Nov 4, 2018

@nkcsgexi How did you get this error? I cannot reproduce it on my machine.

Renaming the product name to something that doesn't include a dash should probably resolve the issue, although I don't quite understand what the issue is.

@ahoppen ahoppen deleted the static-dynamic-product branch November 4, 2018 07:42
@nkcsgexi
Copy link
Contributor

nkcsgexi commented Nov 4, 2018

I can see the error either by invoking Swift-side build script:

swift/utils/build-script --swiftsyntax --skip-test-cmark --skip-test-swift --skip-test-llbuild --skip-test-swiftpm --no-assert --skip-build-benchmarks --verbose -t

or invoking the swift-syntax build script alone:

$./swift-syntax/build-script.py

I'll revert this change for now till we have a good solution for preserving dylib names; plus linking SwiftSyntax statically makes sense to be the default setting for our users for now.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 4, 2018

I was able to reproduce the issue if using swift-build that is bundled with Xcode 10.1 (10B61) but not with SwiftPM built from master. I haven't looked into if this is an issue in SwiftPM that has been fixed but is one of the reasons why I preferred to use a ToT SwiftPM to build SwiftSyntax and not a version of SwiftPM that happens to be installed on the system.

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Nov 5, 2018

yeah, unfortunately using the SwiftPM from the environment is almost a hard requirement for us to submit. We can put a ping on this and wait till SwiftPM from the environment is up to date and then change the Package.swift file to the right configuration.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 8, 2018

Ah, OK. Well. In that case it looks like we can only make this change once the next version of SwiftPM has shipped.

adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
Prevent breaking at the space in `else {` of a `guard` statement.
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.

2 participants