Skip to content

bazel: Update swift_static_framework to use swiftinterfaces#815

Merged
keith merged 4 commits intomasterfrom
ks/bazel-update-swift_static_framework-to-use-swiftinterfaces
May 28, 2020
Merged

bazel: Update swift_static_framework to use swiftinterfaces#815
keith merged 4 commits intomasterfrom
ks/bazel-update-swift_static_framework-to-use-swiftinterfaces

Conversation

@keith
Copy link
Member

@keith keith commented Apr 27, 2020

This is an in between to #797
which starts building the archive with swiftinterface files, but does
it with our existing swift_static_framework rule because of issues
linked on that PR.

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

@keith keith requested a review from rebello95 April 27, 2020 17:15
rebello95
rebello95 previously approved these changes Apr 27, 2020
Copy link
Contributor

@rebello95 rebello95 left a comment

Choose a reason for hiding this comment

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

LGTM, one comment. If you could also update the description to include a bit more direct context from the PR regarding their differences that might be helpful for future reference

@keith keith force-pushed the ks/bazel-update-swift_static_framework-to-use-swiftinterfaces branch from 442da75 to 9bc0078 Compare April 27, 2020 21:20
@stale
Copy link

stale bot commented May 4, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label May 4, 2020
@keith keith force-pushed the ks/bazel-update-swift_static_framework-to-use-swiftinterfaces branch from a96c5f2 to 8e0a0c6 Compare May 7, 2020 20:28
@stale stale bot removed the stale label May 7, 2020
@keith
Copy link
Member Author

keith commented May 7, 2020

This still includes the swiftmodule file in the framework because of this issue, bazelbuild/rules_apple#753 this way people can continue to use it as before for now, but folks who apply this patch can use the swiftinterface instead.

@keith keith force-pushed the ks/bazel-update-swift_static_framework-to-use-swiftinterfaces branch 2 times, most recently from 226f70c to 41cece6 Compare May 18, 2020 22:06
@stale
Copy link

stale bot commented May 25, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label May 25, 2020
keith added 4 commits May 28, 2020 15:18
This is an in between to #797
which starts building the archive with swiftinterface files, but does
it with our existing swift_static_framework rule because of issues
linked on that PR.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith force-pushed the ks/bazel-update-swift_static_framework-to-use-swiftinterfaces branch from 41cece6 to 0d88782 Compare May 28, 2020 22:18
@stale stale bot removed the stale label May 28, 2020
@keith keith marked this pull request as ready for review May 28, 2020 23:45
@keith keith requested a review from rebello95 May 28, 2020 23:45
@keith
Copy link
Member Author

keith commented May 28, 2020

This currently contains both the swiftinterface and the swiftmodule file, once bazelbuild/rules_apple#753 is fixed we can remove the swiftmodule file

@@ -1,4 +1,5 @@
@testable import Envoy
import EnvoyEngine
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, any reason not to use @_implementationOnly here for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't really matter for tests since no one consumes their output swiftmodule, and the annoyance is that if you do it once in a single target you have to do it for all imports, so then everyone has to know about it.

@keith keith merged commit 35ac67a into master May 28, 2020
@keith keith deleted the ks/bazel-update-swift_static_framework-to-use-swiftinterfaces branch May 28, 2020 23:53
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