Skip to content

[cxx-interop] Do not emit C++ interop flag in textual interfaces #77754

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
Dec 6, 2024
Merged

[cxx-interop] Do not emit C++ interop flag in textual interfaces #77754

merged 1 commit into from
Dec 6, 2024

Conversation

egorzhdan
Copy link
Contributor

This makes sure that the compiler does not emit -enable-experimental-cxx-interop/-cxx-interoperability-mode flags in .swiftinterface files. Those flags were breaking explicit module builds. The module can still be rebuilt from its textual interface if C++ interop was enabled in the current compilation.

rdar://140203932

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Nov 21, 2024
@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan requested a review from artemcm November 21, 2024 00:33
@tshortli
Copy link
Contributor

It makes sense to me that when Cxx interop is an implementation detail of a module, but not used in its interface, that the flag should be hidden to avoid affecting the mode in which downstream dependencies are compiled with respect to the client. What about when the interface does depend on Cxx interop, though?

Xazax-hun
Xazax-hun previously approved these changes Nov 21, 2024
View reviewed changes
@Xazax-hun Xazax-hun dismissed their stale review November 21, 2024 11:28

I have not read Allan's comment before reviewing.

@egorzhdan egorzhdan mentioned this pull request Nov 21, 2024
Merged
egorzhdan added a commit to swiftlang/swift-driver that referenced this pull request Nov 21, 2024
@egorzhdan
[cxx-interop] Remove -experimental-cxx-stdlib flag
e4fe1c6 
This change is a counterpart of swiftlang/swift#77754.

rdar://140203932
@egorzhdan
Copy link
Contributor Author

What about when the interface does depend on Cxx interop, though?

If the -cxx-interoperability-flag was passed to the current compiler invocation, it should get propagated to the compiler sub-instance that rebuilds the module from its interface, and the build would succeed. If the flag wasn't passed, the user would see a compiler error when trying to build the module.

The user-facing message isn't ideal, it could explicitly mention the fix of adding the missing flag – we should improve that in the future.

egorzhdan added a commit to swiftlang/swift-driver that referenced this pull request Nov 21, 2024
@egorzhdan
[cxx-interop] Do not emit C++ interop flag in textual interfaces
04d3e8f 
This change is a counterpart of swiftlang/swift#77754.

rdar://140203932
@egorzhdan
Copy link
Contributor Author

@swift-ci please test with following PR:

swiftlang/swift-driver#1737

@tshortli
Copy link
Contributor

The user-facing message isn't ideal, it could explicitly mention the fix of adding the missing flag – we should improve that in the future.

Ok, yeah I'd like to see something like that in the future to make failures to build textual interfaces more explainable and debuggable. For example, we could require that owners of libraries that emit textual interfaces which externally depend on Cxx interop specify a new flag that does get printed in the interface that causes client builds to fail fast if they do not enable interop themselves. The module interface verification build task should be able to diagnose whether this new flag is needed for the library owner.

@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please test

@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch from be4698f to 18d9766 Compare November 21, 2024 20:32
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please test

@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch from 18d9766 to 2ad47af Compare November 22, 2024 00:55
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please test

@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch from 2ad47af to a273999 Compare November 22, 2024 02:02
@egorzhdan egorzhdan enabled auto-merge November 22, 2024 02:03
@egorzhdan egorzhdan disabled auto-merge November 22, 2024 02:11
@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch 3 times, most recently from 3876783 to f2fbfaa Compare November 28, 2024 13:14
@egorzhdan egorzhdan requested a review from xedin as a code owner November 28, 2024 13:14
@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch 7 times, most recently from 26ff7fd to bc35e93 Compare November 29, 2024 21:01
@egorzhdan egorzhdan marked this pull request as draft December 2, 2024 12:50
@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch 3 times, most recently from 2736c8c to b8e641e Compare December 4, 2024 19:02
@egorzhdan egorzhdan marked this pull request as ready for review December 4, 2024 23:09
@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch from b8e641e to b760177 Compare December 5, 2024 13:17
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please test

Xazax-hun
Xazax-hun approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

LGTM!

@egorzhdan
[cxx-interop] Do not emit C++ interop flag in textual interfaces
7ae2beb 
This makes sure that the compiler does not emit `-enable-experimental-cxx-interop`/`-cxx-interoperability-mode` flags in `.swiftinterface` files. Those flags were breaking explicit module builds. The module can still be rebuilt from its textual interface if C++ interop was enabled in the current compilation.

rdar://140203932
@egorzhdan egorzhdan force-pushed the egorzhdan/swiftinterface-no-cxx-flag branch from b760177 to 7ae2beb Compare December 5, 2024 19:25
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please smoke test

egorzhdan added a commit to swiftlang/swift-driver that referenced this pull request Dec 5, 2024
@egorzhdan
[cxx-interop] Do not emit C++ interop flag in textual interfaces
235164e 
This change is a counterpart of swiftlang/swift#77754.

rdar://140203932
(cherry picked from commit 04d3e8f)
@egorzhdan egorzhdan mentioned this pull request Dec 5, 2024
Merged
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please smoke test Linux

2 similar comments
@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please smoke test Linux

@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please smoke test Linux

@egorzhdan
Copy link
Contributor Author

Please test with following PR:

swiftlang/swift-driver#1737

@swift-ci please smoke test macOS

@egorzhdan egorzhdan merged commit 7704534 into main Dec 6, 2024
3 checks passed
@egorzhdan egorzhdan deleted the egorzhdan/swiftinterface-no-cxx-flag branch December 6, 2024 14:30
@egorzhdan egorzhdan mentioned this pull request Dec 17, 2024
Merged
@j-hui j-hui mentioned this pull request Feb 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xazax-hun Xazax-hun Xazax-hun approved these changes

@artemcm artemcm artemcm approved these changes

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@hyp hyp Awaiting requested review from hyp

@tshortli tshortli Awaiting requested review from tshortli tshortli is a code owner

@hborla hborla Awaiting requested review from hborla

@slavapestov slavapestov Awaiting requested review from slavapestov

@xedin xedin Awaiting requested review from xedin

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@egorzhdan @tshortli @Xazax-hun @artemcm