Skip to content

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

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
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
Contributor

@modocache modocache commented Jun 14, 2016 •
edited
Loading

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
Contributor Author

modocache commented Jun 14, 2016

@swift-ci please test

gribozavr
gribozavr reviewed Jun 15, 2016
View reviewed changes
cmake/modules/AddSwift.cmake Outdated
Copy link
Contributor

@gribozavr gribozavr Jun 15, 2016

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
Contributor

gribozavr commented Jun 15, 2016

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
Contributor

gribozavr commented Jun 15, 2016

@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
Contributor Author

modocache commented Jun 15, 2016

@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
Contributor Author

modocache commented Jun 15, 2016

@swift-ci please test

modocache
modocache reviewed Jun 15, 2016
View reviewed changes
stdlib/public/SwiftRemoteMirror/CMakeLists.txt Outdated
Copy link
Contributor Author

@modocache modocache Jun 15, 2016

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
Contributor

@gribozavr gribozavr Jun 16, 2016

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
Contributor Author

modocache commented Jun 16, 2016

@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 swiftlang:master Jun 18, 2016
@gribozavr
Copy link
Contributor

gribozavr commented Jun 18, 2016

@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
Contributor Author

modocache commented Jun 19, 2016

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.

2 participants

@modocache @gribozavr