Skip to content

Commit e9def03

Browse files
gjcairogjcairo
authored
Add CompileTests for InternalImportsByDefault (apple#1709)
* Add CompileTests for InternalImportsByDefault * Update Reference and Makefile * Add missing headers * Update Makefile * Fix references * Fix Makefile * Fix Package.swift * Fix build on 5.8
1 parent beeb414 commit e9def03

File tree

12 files changed

+289
-4
lines changed

12 files changed

+289
-4
lines changed

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ xcbaselines
1111
mined_words.txt
1212
/CompileTests/MultiModule/.build
1313
/CompileTests/MultiModule/.swiftpm
14+
/CompileTests/InternalImportsByDefault/.build
15+
/CompileTests/InternalImportsByDefault/.swiftpm
1416
/*DescriptorTestData.bin
1517
/Package.resolved
1618
/PluginLibEditionDefaults.bin

‎CompileTests/InternalImportsByDefault/Package.swift‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// swift-tools-version: 5.8
2+
3+
// Package.swift
4+
//
5+
// Copyright (c) 2024 Apple Inc. and the project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See LICENSE.txt for license information:
9+
// https://github.com/apple/swift-protobuf/blob/main/LICENSE.txt
10+
11+
import PackageDescription
12+
13+
#if compiler(>=5.9)
14+
let package = Package(
15+
name: "CompileTests",
16+
dependencies: [
17+
.package(path: "../..")
18+
],
19+
targets: [
20+
.executableTarget(
21+
name: "InternalImportsByDefault",
22+
dependencies: [
23+
.product(name: "SwiftProtobuf", package: "swift-protobuf"),
24+
],
25+
exclude: [
26+
"Protos/SomeProtoWithBytes.proto",
27+
"Protos/ServiceOnly.proto"
28+
],
29+
swiftSettings: [
30+
.enableExperimentalFeature("InternalImportsByDefault"),
31+
.enableExperimentalFeature("AccessLevelOnImport"),
32+
// Enable warnings as errors so the build fails if warnings are
33+
// present in generated code.
34+
.unsafeFlags(["-warnings-as-errors"])
35+
],
36+
plugins: [
37+
.plugin(name: "SwiftProtobufPlugin", package: "swift-protobuf")
38+
]
39+
),
40+
]
41+
)
42+
#else
43+
let package = Package(
44+
name: "CompileTests",
45+
targets: [
46+
.executableTarget(
47+
name: "InternalImportsByDefault",
48+
exclude: [
49+
"swift-protobuf-config.json",
50+
"Protos/SomeProtoWithBytes.proto",
51+
"Protos/ServiceOnly.proto"
52+
]
53+
)
54+
]
55+
)
56+
#endif

‎CompileTests/InternalImportsByDefault/README.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# CompileTests/InternalImportsByDefault
2+
3+
This is a test case that ensures that generated code builds correctly when
4+
`InternalImportsByDefault` is enabled and the code is generated with public
5+
visibility.
6+
7+
When support for access level modifiers on imports was first added, an issue
8+
was encountered where publicly-generated protos would generate build errors and
9+
warnings when `InternalImportsByDefault` was enabled, as some dependencies were
10+
imported without an explicit access level modifier (i.e. `Foundation`), and some
11+
where sometimes imported as `public` without actually being used in the
12+
generated code at all (i.e. `Foundation` and `SwiftProtobuf`).

‎CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/ServiceOnly.proto‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// This proto file should generate an empty file, since the plugin will ignore
2+
// service definitions.
3+
// This is here to make sure we don't import Foundation or SwiftProtobuf when
4+
// it's not necessary.
5+
6+
service SomeService {
7+
}

‎CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/SomeProtoWithBytes.proto‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// This proto will generate a Swift file that imports Foundation, because it
2+
// defines a bytes field.
3+
// Because InternalImportsByDefault is enabled on this module and we generate
4+
// protos with public visibility, the build will fail if the access level
5+
// modifier is missing (or wrong) since it will default the import to `internal`
6+
// and cause a conflict of access levels, since the `someBytes` property defined
7+
// on the message will be public.
8+
9+
message SomeProtoWithBytes {
10+
optional bytes someBytes = 2;
11+
optional string ext_str = 100;
12+
}

‎CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/main.swift‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// main.swift
2+
//
3+
// Copyright (c) 2024 Apple Inc. and the project authors
4+
// Licensed under Apache License v2.0 with Runtime Library Exception
5+
//
6+
// See LICENSE.txt for license information:
7+
// https://github.com/apple/swift-protobuf/blob/main/LICENSE.txt
8+
9+
// This test only makes sense for Swift 5.9+ because 5.8 doesn't support access
10+
// level on imports.
11+
#if compiler(>=5.9)
12+
private import Foundation
13+
14+
struct InternalImportsByDefault {
15+
static func main() {
16+
let protoWithBytes = SomeProtoWithBytes.with { proto in
17+
proto.someBytes = Data()
18+
proto.extStr = ""
19+
}
20+
blackhole(protoWithBytes)
21+
}
22+
}
23+
24+
@inline(never)
25+
func blackhole<T>(_: T) {}
26+
#endif

‎CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/swift-protobuf-config.json‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"invocations": [
3+
{
4+
"protoFiles": [
5+
"Protos/SomeProtoWithBytes.proto",
6+
"Protos/ServiceOnly.proto",
7+
],
8+
"visibility": "public",
9+
"useAccessLevelOnImports": true
10+
}
11+
]
12+
}

‎Makefile‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,15 @@ PROTOS_DIRS=Conformance protoc-gen-swiftTests SwiftProtobuf SwiftProtobufPluginL
7979
clean \
8080
compile-tests \
8181
compile-tests-multimodule \
82+
compile-tests-internalimportsbydefault \
8283
default \
8384
docs \
8485
install \
8586
pod-lib-lint \
8687
reference \
8788
regenerate \
8889
regenerate-compiletests-multimodule-protos \
90+
copy-compiletests-internalimportsbydefault-protos \
8991
regenerate-compiletests-protos \
9092
regenerate-conformance-protos \
9193
regenerate-fuzz-protos \
@@ -194,19 +196,33 @@ test-plugin: build ${PROTOC_GEN_SWIFT}
194196
--tfiws_opt=ProtoPathModuleMappings=Protos/CompileTests/MultiModule/module_mappings.pbascii \
195197
--tfiws_out=_test/CompileTests/MultiModule \
196198
`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
199+
@mkdir -p _test/CompileTests/InternalImportsByDefault
200+
${GENERATE_SRCS} \
201+
-I Protos/CompileTests/InternalImportsByDefault \
202+
--tfiws_opt=Visibility=Public \
203+
--tfiws_opt=UseAccessLevelOnImports=true \
204+
--tfiws_out=_test/CompileTests/InternalImportsByDefault \
205+
`(find Protos/CompileTests/InternalImportsByDefault -type f -name "*.proto")`
197206
diff -ru _test Reference
198207

199208
# Test the SPM plugin.
200209
test-spm-plugin:
201210
env PROTOC_PATH=$(shell realpath ${PROTOC}) ${SWIFT} test --package-path PluginExamples
202211

203-
compile-tests: compile-tests-multimodule
212+
compile-tests: \
213+
compile-tests-multimodule \
214+
compile-tests-internalimportsbydefault
204215

205-
# Test that ensure generating public into multiple modules with `import public`
216+
# Test that ensures generating public into multiple modules with `import public`
206217
# yields buildable code.
207218
compile-tests-multimodule:
208219
${SWIFT} test --package-path CompileTests/MultiModule
209220

221+
# Test that ensures that using access level modifiers on imports yields code that's buildable
222+
# when `InternalImportsByDefault` is enabled on the module.
223+
compile-tests-internalimportsbydefault:
224+
env PROTOC_PATH=$(shell realpath ${PROTOC}) ${SWIFT} build --package-path CompileTests/InternalImportsByDefault
225+
210226

211227
# Rebuild the reference files by running the local version of protoc-gen-swift
212228
# against our menagerie of sample protos.
@@ -240,6 +256,13 @@ reference: build ${PROTOC_GEN_SWIFT}
240256
--tfiws_opt=ProtoPathModuleMappings=Protos/CompileTests/MultiModule/module_mappings.pbascii \
241257
--tfiws_out=Reference/CompileTests/MultiModule \
242258
`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
259+
@mkdir -p Reference/CompileTests/InternalImportsByDefault
260+
${GENERATE_SRCS} \
261+
-I Protos/CompileTests/InternalImportsByDefault \
262+
--tfiws_opt=Visibility=Public \
263+
--tfiws_opt=UseAccessLevelOnImports=true \
264+
--tfiws_out=Reference/CompileTests/InternalImportsByDefault \
265+
`(find Protos/CompileTests/InternalImportsByDefault -type f -name "*.proto")`
243266

244267
#
245268
# Rebuild the generated .pb.swift test files by running
@@ -494,10 +517,12 @@ regenerate-conformance-protos: build ${PROTOC_GEN_SWIFT}
494517
`find Protos/Conformance -type f -name "*.proto"`
495518

496519
# Rebuild just the protos used by the CompileTests.
497-
regenerate-compiletests-protos: regenerate-compiletests-multimodule-protos
520+
regenerate-compiletests-protos: \
521+
regenerate-compiletests-multimodule-protos \
522+
copy-compiletests-internalimportsbydefault-protos
498523

499524
# Update the CompileTests/MultiModule files.
500-
# NOTE: Any changes here must be done of the "test-plugin" target so it
525+
# NOTE: Any changes here must also be done on the "test-plugin" target so it
501526
# generates in the same way.
502527
regenerate-compiletests-multimodule-protos: build ${PROTOC_GEN_SWIFT}
503528
find CompileTests/MultiModule -name "*.pb.swift" -exec rm -f {} \;
@@ -508,6 +533,12 @@ regenerate-compiletests-multimodule-protos: build ${PROTOC_GEN_SWIFT}
508533
--tfiws_out=CompileTests/MultiModule \
509534
`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
510535

536+
# We use the plugin for the InternalImportsByDefault test, so we don't actually need to regenerate
537+
# anything. However, to keep the protos centralised in a single place (the Protos directory),
538+
# this simply copies those files to the InternalImportsByDefault package in case they change.
539+
copy-compiletests-internalimportsbydefault-protos:
540+
@cp Protos/CompileTests/InternalImportsByDefault/* CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos
541+
511542
# Helper to check if there is a protobuf checkout as expected.
512543
check-for-protobuf-checkout:
513544
@if [ ! -d "${GOOGLE_PROTOBUF_CHECKOUT}/src/google/protobuf" ]; then \

‎Protos/CompileTests/InternalImportsByDefault/ServiceOnly.proto‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// This proto file should generate an empty file, since the plugin will ignore
2+
// service definitions.
3+
// This is here to make sure we don't import Foundation or SwiftProtobuf when
4+
// it's not necessary.
5+
6+
service SomeService {
7+
}

‎Protos/CompileTests/InternalImportsByDefault/SomeProtoWithBytes.proto‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// This proto will generate a Swift file that imports Foundation, because it
2+
// defines a bytes field.
3+
// Because InternalImportsByDefault is enabled on this module and we generate
4+
// protos with public visibility, the build will fail if the access level
5+
// modifier is missing (or wrong) since it will default the import to `internal`
6+
// and cause a conflict of access levels, since the `someBytes` property defined
7+
// on the message will be public.
8+
9+
message SomeProtoWithBytes {
10+
optional bytes someBytes = 2;
11+
optional string ext_str = 100;
12+
}

0 commit comments

Comments
 (0)