Skip to content

Commit 1fb0d06

Browse files
zhangskzacozzette
zhangskz
and
acozzette
authored
Cherry pick import option for protoc (#21489)
* Move LazilyBuildDependenciesTest into its own .cc file and CMake test binary One of the test cases involves observing the lazy loading behavior of the generated descriptor pool. Since this pool is a global, it can be influenced by other unrelated tests, and whether the test succeeds or fails depends on the test ordering. Moving the test into its own binary ensures that it will pass regardless of test ordering. Fixes #21431. PiperOrigin-RevId: 752313710 * Add support for import option for protoc. PiperOrigin-RevId: 752390262 --------- Co-authored-by: Adam Cozzette <[email protected]>
1 parent 664d94a commit 1fb0d06

13 files changed

+1598
-509
lines changed

‎cmake/tests.cmake

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,26 @@ add_test(NAME full-test
175175
COMMAND tests ${protobuf_GTEST_ARGS}
176176
WORKING_DIRECTORY ${protobuf_SOURCE_DIR})
177177

178+
# This test involves observing and modifying the internal state of the global
179+
# descriptor pool, so it needs to be in its own binary to avoid conflicting
180+
# with other tests.
181+
add_executable(lazily-build-dependencies-test
182+
${lazily_build_dependencies_test_files}
183+
)
184+
185+
target_link_libraries(lazily-build-dependencies-test
186+
libtest_common
187+
libtest_common_lite
188+
${protobuf_LIB_PROTOBUF}
189+
${protobuf_ABSL_USED_TARGETS}
190+
${protobuf_ABSL_USED_TEST_TARGETS}
191+
GTest::gmock_main
192+
)
193+
194+
add_test(NAME lazily-build-dependencies-test
195+
COMMAND lazily-build-dependencies-test ${protobuf_GTEST_ARGS}
196+
WORKING_DIRECTORY ${protobuf_SOURCE_DIR})
197+
178198
if (protobuf_BUILD_LIBUPB)
179199
set(upb_test_proto_genfiles)
180200
foreach(proto_file ${upb_test_protos_files} ${descriptor_proto_proto_srcs})

‎pkg/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ gen_file_lists(
132132
"//upb:test_protos": "upb_test_protos",
133133
"//upb:test_srcs": "upb_test",
134134
"//src/google/protobuf:full_test_srcs": "protobuf_test",
135+
"//src/google/protobuf:lazily_build_dependencies_test_srcs": "lazily_build_dependencies_test",
135136
"//src/google/protobuf:test_proto_all_srcs": "protobuf_test_protos",
136137
"//src/google/protobuf:lite_test_srcs": "protobuf_lite_test",
137138
"//src/google/protobuf:lite_test_proto_srcs": "protobuf_lite_test_protos",

‎src/file_lists.cmake

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1452,6 +1452,11 @@ set(protobuf_test_files
14521452
${protobuf_SOURCE_DIR}/src/google/protobuf/wire_format_unittest.cc
14531453
)
14541454

1455+
# @//src/google/protobuf:lazily_build_dependencies_test_srcs
1456+
set(lazily_build_dependencies_test_files
1457+
${protobuf_SOURCE_DIR}/src/google/protobuf/lazily_build_dependencies_test.cc
1458+
)
1459+
14551460
# @//src/google/protobuf:test_proto_all_srcs
14561461
set(protobuf_test_protos_files
14571462
${protobuf_SOURCE_DIR}/src/google/protobuf/any_test.proto

‎src/google/protobuf/BUILD.bazel

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,6 +1568,30 @@ cc_test(
15681568
],
15691569
)
15701570

1571+
cc_test(
1572+
name = "lazily_build_dependencies_test",
1573+
srcs = ["lazily_build_dependencies_test.cc"],
1574+
copts = COPTS,
1575+
deps = [
1576+
":any_cc_proto",
1577+
":cc_test_protos",
1578+
":descriptor_legacy",
1579+
":port",
1580+
":protobuf",
1581+
":test_textproto",
1582+
":unittest_proto3_arena_cc_proto",
1583+
"//src/google/protobuf/io",
1584+
"//src/google/protobuf/io:tokenizer",
1585+
"//src/google/protobuf/stubs",
1586+
"//src/google/protobuf/testing",
1587+
"//src/google/protobuf/testing:file",
1588+
"@abseil-cpp//absl/strings",
1589+
"@abseil-cpp//absl/strings:str_format",
1590+
"@googletest//:gtest",
1591+
"@googletest//:gtest_main",
1592+
],
1593+
)
1594+
15711595
cc_library(
15721596
name = "test_textproto",
15731597
testonly = True,
@@ -2353,13 +2377,20 @@ filegroup(
23532377
"*unittest.cc",
23542378
],
23552379
exclude = [
2380+
"lazily_build_dependencies_test.cc",
23562381
"lite_unittest.cc",
23572382
"lite_arena_unittest.cc",
23582383
],
23592384
),
23602385
visibility = ["//pkg:__pkg__"],
23612386
)
23622387

2388+
filegroup(
2389+
name = "lazily_build_dependencies_test_srcs",
2390+
srcs = ["lazily_build_dependencies_test.cc"],
2391+
visibility = ["//pkg:__pkg__"],
2392+
)
2393+
23632394
filegroup(
23642395
name = "lite_test_srcs",
23652396
srcs = [

‎src/google/protobuf/compiler/command_line_interface.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,6 +1248,7 @@ int CommandLineInterface::Run(int argc, const char* const argv[]) {
12481248
}
12491249

12501250
descriptor_pool->EnforceWeakDependencies(true);
1251+
descriptor_pool->EnforceOptionDependencies(true);
12511252
descriptor_pool->EnforceNamingStyle(true);
12521253

12531254
if (!SetupFeatureResolution(*descriptor_pool)) {

‎src/google/protobuf/compiler/command_line_interface_unittest.cc

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,7 @@ TEST_F(CommandLineInterfaceTest, MultipleInputs_UnusedImport_DescriptorSetIn) {
675675

676676
file_descriptor_proto = file_descriptor_set.add_file();
677677
file_descriptor_proto->set_name("import_custom_unknown_options.proto");
678-
file_descriptor_proto->add_dependency("custom_options.proto");
678+
file_descriptor_proto->add_option_dependency("custom_options.proto");
679679
// Add custom message option to unknown field. This custom option is
680680
// not known in generated pool, thus option will be in unknown fields.
681681
file_descriptor_proto->add_message_type()->set_name("Bar");
@@ -747,6 +747,49 @@ TEST_F(CommandLineInterfaceTest, MultipleInputsWithImport) {
747747
"bar.proto", "Bar");
748748
}
749749

750+
TEST_F(CommandLineInterfaceTest, MultipleInputsWithOptionImport) {
751+
// Test parsing multiple input files with an option import of a separate file.
752+
CreateTempFile("google/protobuf/descriptor.proto",
753+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
754+
CreateTempFile("foo.proto",
755+
R"schema(
756+
edition = "2024";
757+
message Foo {}
758+
)schema");
759+
CreateTempFile("bar.proto",
760+
R"schema(
761+
edition = "2024";
762+
import option "baz.proto";
763+
message Bar {
764+
Bar a = 1 [(field_options) = { baz: 1 }];
765+
}
766+
)schema");
767+
CreateTempFile("baz.proto",
768+
R"schema(
769+
edition = "2024";
770+
import "google/protobuf/descriptor.proto";
771+
message Baz {
772+
int64 baz = 1;
773+
}
774+
extend google.protobuf.FieldOptions {
775+
Baz field_options = 5000;
776+
}
777+
)schema");
778+
779+
Run("protocol_compiler --test_out=$tmpdir --plug_out=$tmpdir "
780+
"--proto_path=$tmpdir foo.proto bar.proto --experimental_editions");
781+
782+
ExpectNoErrors();
783+
ExpectGeneratedWithMultipleInputs("test_generator", "foo.proto,bar.proto",
784+
"foo.proto", "Foo");
785+
ExpectGeneratedWithMultipleInputs("test_generator", "foo.proto,bar.proto",
786+
"bar.proto", "Bar");
787+
ExpectGeneratedWithMultipleInputs("test_plugin", "foo.proto,bar.proto",
788+
"foo.proto", "Foo");
789+
ExpectGeneratedWithMultipleInputs("test_plugin", "foo.proto,bar.proto",
790+
"bar.proto", "Bar");
791+
}
792+
750793

751794
TEST_F(CommandLineInterfaceTest, MultipleInputsWithImport_DescriptorSetIn) {
752795
// Test parsing multiple input files with an import of a separate file.
@@ -1434,6 +1477,35 @@ TEST_F(CommandLineInterfaceTest, FeatureExtensions) {
14341477
ExpectNoErrors();
14351478
}
14361479

1480+
TEST_F(CommandLineInterfaceTest, ImportOptions) {
1481+
CreateTempFile("google/protobuf/descriptor.proto",
1482+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
1483+
CreateTempFile("options.proto",
1484+
R"schema(
1485+
syntax = "proto2";
1486+
package test;
1487+
import "google/protobuf/descriptor.proto";
1488+
extend google.protobuf.FileOptions {
1489+
optional TestOptions opt = 99990;
1490+
}
1491+
message TestOptions {
1492+
repeated int32 a = 1;
1493+
}
1494+
)schema");
1495+
CreateTempFile("foo.proto",
1496+
R"schema(
1497+
edition = "2024";
1498+
import option "options.proto";
1499+
1500+
option (test.opt).a = 1;
1501+
option (.test.opt).a = 2;
1502+
)schema");
1503+
1504+
Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto "
1505+
"--experimental_editions");
1506+
ExpectNoErrors();
1507+
}
1508+
14371509
TEST_F(CommandLineInterfaceTest, FeatureValidationError) {
14381510
CreateTempFile("foo.proto",
14391511
R"schema(
@@ -2672,6 +2744,51 @@ TEST_F(CommandLineInterfaceTest, WriteTransitiveDescriptorSetWithSourceInfo) {
26722744
EXPECT_TRUE(descriptor_set.file(1).has_source_code_info());
26732745
}
26742746

2747+
TEST_F(CommandLineInterfaceTest, NoWriteTransitiveOptionImportDescriptorSet) {
2748+
CreateTempFile("google/protobuf/descriptor.proto",
2749+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
2750+
CreateTempFile("custom_option.proto",
2751+
R"schema(
2752+
syntax = "proto2";
2753+
import "google/protobuf/descriptor.proto";
2754+
extend .google.protobuf.FileOptions {
2755+
optional int32 file_opt = 5000;
2756+
}
2757+
)schema");
2758+
CreateTempFile("foo.proto",
2759+
R"schema(
2760+
syntax = "proto2";
2761+
message Foo {}
2762+
)schema");
2763+
CreateTempFile("bar.proto",
2764+
R"schema(
2765+
edition = "2024";
2766+
import "foo.proto";
2767+
import option "custom_option.proto";
2768+
option (file_opt) = 1;
2769+
message Bar {
2770+
Foo foo = 1;
2771+
}
2772+
)schema");
2773+
2774+
Run("protocol_compiler --descriptor_set_out=$tmpdir/descriptor_set "
2775+
"--include_imports --proto_path=$tmpdir bar.proto "
2776+
"--experimental_editions");
2777+
2778+
ExpectNoErrors();
2779+
2780+
FileDescriptorSet descriptor_set;
2781+
ReadDescriptorSet("descriptor_set", &descriptor_set);
2782+
if (HasFatalFailure()) return;
2783+
EXPECT_EQ(2, descriptor_set.file_size());
2784+
if (descriptor_set.file(0).name() == "bar.proto") {
2785+
std::swap(descriptor_set.mutable_file()->mutable_data()[0],
2786+
descriptor_set.mutable_file()->mutable_data()[1]);
2787+
}
2788+
EXPECT_EQ("foo.proto", descriptor_set.file(0).name());
2789+
EXPECT_EQ("bar.proto", descriptor_set.file(1).name());
2790+
}
2791+
26752792
TEST_F(CommandLineInterfaceTest, DescriptorSetOptionRetention) {
26762793
// clang-format off
26772794
CreateTempFile(

‎src/google/protobuf/compiler/parser.cc

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,7 @@ bool Parser::ParseTopLevelStatement(FileDescriptorProto* file,
731731
FileDescriptorProto::kMessageTypeFieldNumber, location, file);
732732
} else if (LookingAt("import")) {
733733
return ParseImport(file->mutable_dependency(),
734+
file->mutable_option_dependency(),
734735
file->mutable_public_dependency(),
735736
file->mutable_weak_dependency(), root_location, file);
736737
} else if (LookingAt("package")) {
@@ -2498,16 +2499,42 @@ bool Parser::ParsePackage(FileDescriptorProto* file,
24982499
}
24992500

25002501
bool Parser::ParseImport(RepeatedPtrField<std::string>* dependency,
2502+
RepeatedPtrField<std::string>* option_dependency,
25012503
RepeatedField<int32_t>* public_dependency,
25022504
RepeatedField<int32_t>* weak_dependency,
25032505
const LocationRecorder& root_location,
25042506
const FileDescriptorProto* containing_file) {
2505-
LocationRecorder location(root_location,
2506-
FileDescriptorProto::kDependencyFieldNumber,
2507-
dependency->size());
2508-
2507+
io::Tokenizer::Token import_start = input_->current();
25092508
DO(Consume("import"));
2509+
std::string import_file;
2510+
if (LookingAt("option")) {
2511+
LocationRecorder option_import_location(
2512+
root_location, FileDescriptorProto::kOptionDependencyFieldNumber,
2513+
option_dependency->size());
2514+
option_import_location.StartAt(import_start);
2515+
if (edition_ < Edition::EDITION_2024) {
2516+
RecordError("option import is not supported before edition 2024.");
2517+
}
2518+
DO(Consume("option"));
2519+
DO(ConsumeString(&import_file,
2520+
"Expected a string naming the file to import."));
2521+
*option_dependency->Add() = import_file;
2522+
2523+
option_import_location.RecordLegacyImportLocation(containing_file,
2524+
import_file);
2525+
DO(ConsumeEndOfDeclaration(";", &option_import_location));
2526+
return true;
2527+
}
25102528

2529+
LocationRecorder import_location(root_location,
2530+
FileDescriptorProto::kDependencyFieldNumber,
2531+
dependency->size());
2532+
import_location.StartAt(import_start);
2533+
if (!option_dependency->empty()) {
2534+
RecordError(
2535+
"imports should precede any option imports to ensure proto files "
2536+
"can roundtrip.");
2537+
}
25112538
if (LookingAt("public")) {
25122539
LocationRecorder public_location(
25132540
root_location, FileDescriptorProto::kPublicDependencyFieldNumber,
@@ -2522,15 +2549,12 @@ bool Parser::ParseImport(RepeatedPtrField<std::string>* dependency,
25222549
DO(Consume("weak"));
25232550
*weak_dependency->Add() = dependency->size();
25242551
}
2525-
2526-
std::string import_file;
25272552
DO(ConsumeString(&import_file,
25282553
"Expected a string naming the file to import."));
25292554
*dependency->Add() = import_file;
2530-
location.RecordLegacyImportLocation(containing_file, import_file);
2531-
2532-
DO(ConsumeEndOfDeclaration(";", &location));
25332555

2556+
import_location.RecordLegacyImportLocation(containing_file, import_file);
2557+
DO(ConsumeEndOfDeclaration(";", &import_location));
25342558
return true;
25352559
}
25362560

@@ -2540,7 +2564,7 @@ bool Parser::ParseVisibility(const FileDescriptorProto* containing_file,
25402564
return false;
25412565
}
25422566

2543-
// Bail out of visiblity checks if < 2024
2567+
// Bail out of visibility checks if < 2024
25442568
if (containing_file->edition() <= Edition::EDITION_2023) {
25452569
return true;
25462570
}

‎src/google/protobuf/compiler/parser.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "google/protobuf/descriptor.pb.h"
2626
#include "google/protobuf/io/tokenizer.h"
2727
#include "google/protobuf/repeated_field.h"
28+
#include "google/protobuf/repeated_ptr_field.h"
2829

2930
// Must be included last.
3031
#include "google/protobuf/port_def.inc"
@@ -347,6 +348,7 @@ class PROTOBUF_EXPORT Parser final {
347348
const LocationRecorder& root_location,
348349
const FileDescriptorProto* containing_file);
349350
bool ParseImport(RepeatedPtrField<std::string>* dependency,
351+
RepeatedPtrField<std::string>* option_dependency,
350352
RepeatedField<int32_t>* public_dependency,
351353
RepeatedField<int32_t>* weak_dependency,
352354
const LocationRecorder& root_location,

0 commit comments

Comments
 (0)