Skip to content

Add static library configuration for _InternalSwiftSyntaxParser #36151

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

Conversation

keith
Copy link
Member

@keith keith commented Feb 25, 2021

When building command line tools that depend on SwiftSyntax, statically
linking this library can be preferred to produce a more portable binary
that doesn't directly depend on the system installation. Currently you
can achieve this benefit by shipping the dylib with your tool, and
editing the rpaths to prefer yours, but that's a lot of trouble.

Fixes https://bugs.swift.org/browse/SR-14001 and probably https://bugs.swift.org/browse/SR-9038

When building command line tools that depend on SwiftSyntax, statically
linking this library can be preferred to produce a more portable binary
that doesn't directly depend on the system installation. Currently you
can achieve this benefit by shipping the dylib with your tool, and
editing the rpaths to prefer yours, but that's a lot of trouble.
@@ -10,14 +10,24 @@ set(LLVM_EXPORTED_SYMBOL_FILE
add_swift_host_library(libSwiftSyntaxParser SHARED
c-include-check.c
libSwiftSyntaxParser.cpp)

add_swift_host_library(libSwiftSyntaxParserStatic STATIC
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel great about all this duplication, but also since only ~half of the things here applied to this, I went with it over creating a function. I'm not a cmake expert so I'd appreciate input!

endif()

add_dependencies(parser-lib libSwiftSyntaxParser)
add_dependencies(parser-lib libSwiftSyntaxParserStatic)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what adding this as a dependency here is for

swift_install_in_component(TARGETS libSwiftSyntaxParser
ARCHIVE DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}" COMPONENT parser-lib
LIBRARY DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}" COMPONENT parser-lib
RUNTIME DESTINATION "bin" COMPONENT parser-lib)
swift_install_in_component(TARGETS libSwiftSyntaxParserStatic
ARCHIVE DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}" COMPONENT parser-lib
LIBRARY DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}" COMPONENT parser-lib
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hoping this translates to being included in produced toolchains so it could be shipped with releases, but I'm not sure if I should build one of those to see, or if it needs to also be added below

@dduan
Copy link
Contributor

dduan commented Feb 25, 2021

@swift-ci please test

@keith
Copy link
Member Author

keith commented Feb 26, 2021

Can you trigger the toolchain build just so we can see how this affects that?

@dduan
Copy link
Contributor

dduan commented Feb 26, 2021

@swift-ci Please Build Toolchain Linux Platform

@dduan
Copy link
Contributor

dduan commented Feb 26, 2021

@swift-ci Please Build Toolchain macOS Platform

@keith
Copy link
Member Author

keith commented Feb 26, 2021

Hrm so I just discovered that I don't think this is enough to be portable, we actually need a "fully linked" static library (bazel terminology, I can't find an analogy in cmake) where all the dependent libraries are included in this output as well.

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 6169a64

Install command
tar zxf swift-PR-36151-563-ubuntu16.04.tar.gz
More info

@keith
Copy link
Member Author

keith commented Feb 28, 2021

So I found a hack that works here where I pretend the library is a dynamic library, and then manually pass -r to the linker. I'm quite surprised that I can't find a way to make cmake produce a distributable static library though so I'd love to hear from some cmake experts if I'm missing something

@MaxDesiatov MaxDesiatov requested a review from akyrtzi March 4, 2021 17:56
@cynicer
Copy link

cynicer commented Jul 6, 2021

I'd be really interested how fully linking of the static libs works as well. Maybe some can help out here? :)

@keith
Copy link
Member Author

keith commented Nov 9, 2021

I got a bit further by using a custom target to combine libraries, but that doesn't play well with the rest of the build system that expects some specific types of targets.

Related cmake feature request https://gitlab.kitware.com/cmake/cmake/-/issues/20858

@keith
Copy link
Member Author

keith commented Jun 7, 2022

#59167

@keith keith closed this Jun 7, 2022
@keith keith deleted the ks/add-static-library-configuration-for-_internalswiftsyntaxparser branch June 7, 2022 23:56
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