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

Enable MemberImportVisibility check on all targets #2142

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

rnro
Copy link
Collaborator

@rnro rnro commented Dec 9, 2024

Enable MemberImportVisibility check on all targets. Use a standard string header and footer to bracket the new block for ease of updating in the future with scripts.

@rnro rnro added the semver/none No version bump required. label Dec 9, 2024
linux_6_0_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
linux_nightly_6_0_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
linux_nightly_main_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
linux_6_0_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable -Xswiftc -enable-upcoming-feature -Xswiftc MemberImportVisibility"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we can't just add this to our default settings in the manifest?

let defaultSwiftSettings: [SwiftSetting] = [

@rnro rnro force-pushed the enable_MemberImportVisibility_check branch from f6d98dc to fe42d6f Compare December 10, 2024 10:35
@rnro rnro changed the title Enable MemberImportVisibility check on 6.0+ pipelines Enable MemberImportVisibility check on all targets Dec 10, 2024
Package.swift Outdated
Comment on lines 121 to 129
for target in package.targets {
var settings = target.swiftSettings ?? []
settings.append(.enableUpcomingFeature("MemberImportVisibility"))
target.swiftSettings = settings
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just add this directly to the defaultSwiftSettings above so that they're all in the same place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intention here is that many of the core Swift on Server repositories have a block of their package manifest which a common structure which can be easily be modified with a script. This block would be used to insert settings which we wish to apply across all repositories.

@rnro rnro force-pushed the enable_MemberImportVisibility_check branch from fe42d6f to 8450297 Compare December 10, 2024 10:43
@rnro rnro force-pushed the enable_MemberImportVisibility_check branch from 8450297 to 0ecbb57 Compare December 11, 2024 09:18
@glbrntt glbrntt added 🔨 semver/patch No public API change. and removed semver/none No version bump required. labels Dec 11, 2024
@rnro rnro force-pushed the enable_MemberImportVisibility_check branch 2 times, most recently from 0b4cf0e to c1c96ac Compare December 11, 2024 11:36
@rnro rnro force-pushed the enable_MemberImportVisibility_check branch from c1c96ac to 1012986 Compare December 11, 2024 12:02
@glbrntt glbrntt merged commit 7ed6f7d into grpc:main Dec 11, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants