-
Notifications
You must be signed in to change notification settings - Fork 104
Remove @_implementationOnly imports guarded by 'SWT_BUILDING_WITH_CMAKE' #554
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
Remove @_implementationOnly imports guarded by 'SWT_BUILDING_WITH_CMAKE' #554
Conversation
@swift-ci please test |
@jeffdav and @compnerd, does my paragraph in the PR description above trying to reconstruct the history seem correct to you? Do you recall any other problems you originally encountered, besides Library Evolution, which led you to need these changes to these I know there was also some discussion about |
@@ -16,9 +16,6 @@ add_compile_options( | |||
"SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -enable-upcoming-feature -Xfrontend ExistentialAny>" | |||
"SHELL:$<$<COMPILE_LANGUAGE:Swift>:-Xfrontend -enable-upcoming-feature -Xfrontend InternalImportsByDefault>") | |||
|
|||
# Definitions applied unconditionally to files of all languages, in all targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this in actually, as it may come in handy in the future that we're able to distinguish CMake from SwiftPM and xcodeproj builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but in retrospect I think this sort of pattern would be better accomplished with feature-scoped guards. For example, I think this one could have been phrased as SWT_NO_ACCESS_LEVEL_IMPORTS
, matching some of our other build time conditionals. So I will proceed with removing this, and we can easily add back other, more specific conditions in the future the need arises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the description seems accurate and complete to me.
This removes the
SWT_BUILDING_WITH_CMAKE
compilation condition, and the@_implementationOnly import _TestingInternals
declarations it guards.Motivation:
When support for building via CMake was first added (in #387), many source files in the
Testing
target were modified to avoid usingprivate
- orinternal
-level imports of the_TestingInternals
module and instead use the older@_implementationOnly import
style when building via CMake. As a result, many files have:This has proven to be a maintenance problem for us, because as new files are added over time, it's not immediately obvious that they need to use this pattern for importing this module—leading to warnings when building with CMake such as the one fixed by #553.
I wasn't aware of the reasoning behind the above change at the time, but I investigated it recently and here's what I concluded: Early on, the changes in #387 did not enable Library Evolution (LE) for the
Testing
module, meaning it was still a non-resilient module. SE-0409 states that all dependencies of non-resilient modules must be loaded by clients, so it would make sense that under those conditions, the build of a client who importsTesting
would fail because clients do not have visibility to the_TestingInternals
module. However, in a later PR commit LE was enabled, but nobody realized that thoseimport
code changes were no longer necessary and could be reverted.I was able to reproduce this and confirm the theory: I added an example client target in the CMake build, then disabled LE in the
Testing
module, and attempted to build the client. It failed with the expected missing module error. Then, re-enabling LE and rebuilding everything, the error went away. Hopefully this is sufficient proof that reverting this is safe, but I'll check with the main contributor of that PR to check and see if there were any other issues before landing.But overall, the goal here is to simplify these imports to reduce maintenance burden and fully embrace SE-0409.
Checklist: