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.

Sign up for GitHub

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

Jump to bottom

[SR-1738] add_swift_library takes SHARED/STATIC arg #3020

Merged
merged 1 commit into from Jun 18, 2016
Merged

[SR-1738] add_swift_library takes SHARED/STATIC arg #3020

merged 1 commit into from Jun 18, 2016

Conversation

modocache
Copy link
Collaborator

@modocache modocache commented Jun 14, 2016 •
edited

What's in this pull request?

As a first step to allowing the build script to build only static library versions of the stdlib (see #2431), change add_swift_library such that callers must pass in SHARED, STATIC, or OBJECT_LIBRARY.

Ideally, only these flags would be used to determine whether to build shared, static, or object libraries, but that is not currently the case -- add_swift_library also checks whether the library IS_STDLIB before performing certain additional actions. This will be cleaned up in a future commit.

/cc @gribozavr

Resolved Related bug number: (SR-1738)

Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@modocache modocache force-pushed the sr-1738-add-swift-library-shared-static-args branch from a24c349 to d2b273a Compare June 14, 2016 22:45
@modocache
Copy link
Collaborator Author

@swift-ci please test

gribozavr
gribozavr reviewed Jun 15, 2016
View reviewed changes
cmake/modules/AddSwift.cmake
@@ -473,6 +477,11 @@ function(_add_swift_library_single target name)
if("${SWIFTLIB_SINGLE_INSTALL_IN_COMPONENT}" STREQUAL "")
message(FATAL_ERROR "INSTALL_IN_COMPONENT is required")
endif()

if(NOT SWIFTLIB_SINGLE_SHARED AND NOT SWIFTLIB_SINGLE_STATIC AND NOT SWIFTLIB_SINGLE_OBJECT_LIBRARY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

80 columns, please.

@gribozavr
Copy link
Collaborator

LGTM, but I wanted to ask you what is your long-term plan about ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} (and same for SDK overlay). Having to expand this variable everywhere seems boilerplate-y. Could we do something smarter, for example, infer the defaults if IS_STDLIB but none of the explicit keywords are specified, and if any keywords are specified, those would override the defaults?

@gribozavr
Copy link
Collaborator

@modocache OS X error:

CMake Error at cmake/modules/AddSwift.cmake:1121 (message):
  Either SHARED, STATIC, or OBJECT_LIBRARY must be specified
Call Stack (most recent call first):
  stdlib/public/SDK/Intents/CMakeLists.txt:1 (add_swift_library)

@modocache modocache force-pushed the sr-1738-add-swift-library-shared-static-args branch from d2b273a to dc0b754 Compare June 15, 2016 13:24
@modocache
Copy link
Collaborator Author

@gribozavr Yes, that sounds like a good idea -- at some point I'll infer defaults based on IS_STDLIB and IS_SDK_OVERLAY.

I fixed the Intents SDK error -- my bad, I was working on an older branch, and on Linux. I also caught a new XPC SDK overlay was added (cool!), so I added the expansion to that call as well. Hopefully OS X tests will pass this time!

@modocache modocache force-pushed the sr-1738-add-swift-library-shared-static-args branch 2 times, most recently from f53588c to 2f18aa1 Compare June 15, 2016 16:29
@modocache
Copy link
Collaborator Author

@swift-ci please test

modocache
modocache reviewed Jun 15, 2016
View reviewed changes
stdlib/public/SwiftRemoteMirror/CMakeLists.txt
@@ -1,4 +1,4 @@
add_swift_library(swiftRemoteMirror SHARED TARGET_LIBRARY DONT_EMBED_BITCODE
add_swift_library(swiftRemoteMirror ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} TARGET_LIBRARY DONT_EMBED_BITCODE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gribozavr You mentioned we always want libswiftRemoteMirror to be built as a shared library. Does this mean this should be hardcoded as SHARED, and only built when SWIFT_BUILD_STDLIB is turned on?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean this should be hardcoded as SHARED, and only built when SWIFT_BUILD_STDLIB is turned on?

Yes to both, good catch!

@modocache modocache mentioned this pull request Jun 15, 2016
Merged
1 task
@modocache
[SR-1738] add_swift_library takes SHARED/STATIC arg
328de9e 
As a first step to allowing the build script to build *only*
static library versions of the stdlib, change `add_swift_library`
such that callers must pass in `SHARED`, `STATIC`, or `OBJECT_LIBRARY`.

Ideally, only these flags would be used to determine whether to
build shared, static, or object libraries, but that is not currently
the case -- `add_swift_library` also checks whether the library
`IS_STDLIB` before performing certain additional actions. This will be
cleaned up in a future commit.
@modocache modocache force-pushed the sr-1738-add-swift-library-shared-static-args branch from 2f18aa1 to 328de9e Compare June 16, 2016 17:16
@modocache
Copy link
Collaborator Author

@gribozavr Addressed the libswiftRemoteMirror issue. Thanks! Is this good to merge if the tests pass?

@swift-ci please test

@modocache modocache mentioned this pull request Jun 16, 2016
Closed
1 task
@gribozavr gribozavr merged commit 5dd3ce9 into apple:master Jun 18, 2016
@gribozavr
Copy link
Collaborator

@modocache Thank you, merged!

@modocache modocache deleted the sr-1738-add-swift-library-shared-static-args branch June 19, 2016 04:27
@modocache
Copy link
Collaborator Author

Awesome, thanks!!

@modocache modocache mannequin mentioned this pull request Jun 14, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@modocache @gribozavr