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.

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

Add CompileTests for InternalImportsByDefault #1709

Merged

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Sep 3, 2024

This PR adds a new CompileTest for checking that generated code builds successfully when InternalImportsByDefault is enabled.

@@ -22,7 +22,7 @@ let package = Package(
.product(name: "SwiftProtobuf", package: "swift-protobuf"),
],
swiftSettings: [
.enableUpcomingFeature("InternalImportsByDefault"),
.enableExperimentalFeature("InternalImportsByDefault"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to use AccessLevelOnImport and not use InternalImportsByDefault, the original issue with Foundation was because we didn't have an access level, so we don't want any level by default, but to ensure we're always providing level and to enable the support on these 5.x compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I forgot to enable AccessLevelOnImport too. If InternalImportsByDefault is not enabled, we still get some default: it defaults to public instead. I came across this bug when having both features enabled.

There were also some issues with 5.8 not understanding these flags so I had to add some compiler guards but now finally CI is happy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What imports are still defaulting wrong? Do we need to fix those?

@gjcairo gjcairo force-pushed the internal-imports-by-default-compile-tests branch from 15882c6 to dad56cb Compare September 4, 2024 13:24
@thomasvl thomasvl merged commit e9def03 into apple:main Sep 5, 2024
10 checks passed
@gjcairo gjcairo deleted the internal-imports-by-default-compile-tests branch September 5, 2024 12:39
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