Skip to content

Commit 9ef9e80

Browse files
mkruskal-googlecopybara-github
mkruskal-google
authored and
copybara-github
committed
Fix import option handling when include_imports isn't set.
These should be handled like regular dependencies and not included in the output FileDescriptorSet. They're not useful on their own, and can be harmful by signaling files that aren't actually direct sources. Also allow descriptor set inputs with missing option dependencies. We won't *output* this, but we should honor it as an input since the option dependencies aren't really necessary. Notably, our kotlin blazel rules pass these stripped descriptor sets to protoc. PiperOrigin-RevId: 822652169
1 parent 793dbb9 commit 9ef9e80

File tree

3 files changed

+200
-29
lines changed

3 files changed

+200
-29
lines changed

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -337,10 +337,12 @@ void CommandLineInterface::GetTransitiveDependencies(
337337
for (int i = 0; i < file->option_dependency_count(); ++i) {
338338
const FileDescriptor* dep =
339339
file->pool()->FindFileByName(file->option_dependency_name(i));
340-
ABSL_CHECK(dep != nullptr)
340+
ABSL_CHECK(dep != nullptr || !descriptor_set_in_names_.empty())
341341
<< "Option dependency " << file->option_dependency_name(i)
342342
<< " not found in pool. This should never happen.";
343-
GetTransitiveDependencies(dep, already_seen, output, options);
343+
if (dep != nullptr) {
344+
GetTransitiveDependencies(dep, already_seen, output, options);
345+
}
344346
}
345347

346348
// Add this file.
@@ -1362,7 +1364,7 @@ int CommandLineInterface::Run(int argc, const char* const argv[]) {
13621364
}
13631365

13641366
descriptor_pool->EnforceWeakDependencies(true);
1365-
descriptor_pool->EnforceOptionDependencies(true);
1367+
descriptor_pool->EnforceOptionDependencies(descriptor_set_in_names_.empty());
13661368
descriptor_pool->EnforceSymbolVisibility(true);
13671369
descriptor_pool->EnforceNamingStyle(true);
13681370

@@ -3216,6 +3218,15 @@ bool CommandLineInterface::WriteDescriptorSet(
32163218
already_seen.insert(dependency);
32173219
}
32183220
}
3221+
for (int j = 0; j < file->option_dependency_count(); j++) {
3222+
const FileDescriptor* dependency =
3223+
file->pool()->FindFileByName(file->option_dependency_name(j));
3224+
// if the dependency isn't in parsed files, mark it as already seen
3225+
if (dependency != nullptr &&
3226+
to_output.find(dependency) == to_output.end()) {
3227+
already_seen.insert(dependency);
3228+
}
3229+
}
32193230
}
32203231
}
32213232
TransitiveDependencyOptions options;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ struct TransitiveDependencyOptions {
5858
bool include_json_name = false;
5959
bool include_source_code_info = false;
6060
bool retain_options = false;
61+
bool skip_dependencies = false;
6162
};
6263

6364
// This class implements the command-line interface to the protocol compiler.

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

Lines changed: 185 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "absl/strings/str_cat.h"
2424
#include "absl/types/span.h"
2525
#include "google/protobuf/compiler/command_line_interface_tester.h"
26+
#include "google/protobuf/cpp_features.pb.h"
2627
#include "google/protobuf/unittest_features.pb.h"
2728
#include "google/protobuf/unittest_invalid_features.pb.h"
2829

@@ -2978,6 +2979,190 @@ TEST_F(CommandLineInterfaceTest, WriteDescriptorSet) {
29782979
EXPECT_TRUE(descriptor_set.file(0).message_type(0).field(0).has_json_name());
29792980
}
29802981

2982+
TEST_F(CommandLineInterfaceTest, ImportOption_DescriptorSetOut) {
2983+
CreateTempFile("google/protobuf/descriptor.proto",
2984+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
2985+
CreateTempFile("custom_option.proto", R"schema(
2986+
edition = "2024";
2987+
import "google/protobuf/descriptor.proto";
2988+
extend google.protobuf.FileOptions {
2989+
int32 file_opt = 50000;
2990+
}
2991+
)schema");
2992+
CreateTempFile("foo.proto", R"schema(
2993+
edition = "2024";
2994+
import option "custom_option.proto";
2995+
option (file_opt) = 99;
2996+
message Foo {}
2997+
)schema");
2998+
2999+
Run("protocol_compiler --descriptor_set_out=$tmpdir/descriptor_set "
3000+
"--proto_path=$tmpdir foo.proto");
3001+
3002+
ExpectNoErrors();
3003+
3004+
FileDescriptorSet descriptor_set;
3005+
ReadDescriptorSet("descriptor_set", &descriptor_set);
3006+
ASSERT_FALSE(HasFatalFailure());
3007+
ASSERT_EQ(descriptor_set.file_size(), 1);
3008+
EXPECT_EQ(descriptor_set.file(0).name(), "foo.proto");
3009+
// Descriptor set should not have source code info.
3010+
EXPECT_FALSE(descriptor_set.file(0).has_source_code_info());
3011+
// Descriptor set should have custom options.
3012+
ASSERT_EQ(descriptor_set.file(0).options().unknown_fields().field_count(), 1);
3013+
EXPECT_EQ(descriptor_set.file(0).options().unknown_fields().field(0).number(),
3014+
50000);
3015+
EXPECT_EQ(descriptor_set.file(0).options().unknown_fields().field(0).varint(),
3016+
99);
3017+
}
3018+
3019+
TEST_F(CommandLineInterfaceTest, ImportOption_DescriptorSetOut_IncludeImports) {
3020+
CreateTempFile("google/protobuf/descriptor.proto",
3021+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
3022+
CreateTempFile("custom_option.proto", R"schema(
3023+
edition = "2024";
3024+
import "google/protobuf/descriptor.proto";
3025+
extend google.protobuf.FileOptions {
3026+
int32 file_opt = 50000;
3027+
}
3028+
)schema");
3029+
CreateTempFile("foo.proto", R"schema(
3030+
edition = "2024";
3031+
import option "custom_option.proto";
3032+
option (file_opt) = 99;
3033+
message Foo {}
3034+
)schema");
3035+
3036+
Run("protocol_compiler --descriptor_set_out=$tmpdir/descriptor_set "
3037+
"--proto_path=$tmpdir foo.proto --include_imports");
3038+
3039+
ExpectNoErrors();
3040+
3041+
FileDescriptorSet descriptor_set;
3042+
ReadDescriptorSet("descriptor_set", &descriptor_set);
3043+
ASSERT_FALSE(HasFatalFailure());
3044+
ASSERT_EQ(descriptor_set.file_size(), 3);
3045+
EXPECT_EQ(descriptor_set.file(0).name(), "google/protobuf/descriptor.proto");
3046+
EXPECT_EQ(descriptor_set.file(1).name(), "custom_option.proto");
3047+
EXPECT_EQ(descriptor_set.file(2).name(), "foo.proto");
3048+
// Descriptor set should have custom options.
3049+
ASSERT_EQ(descriptor_set.file(2).options().unknown_fields().field_count(), 1);
3050+
EXPECT_EQ(descriptor_set.file(2).options().unknown_fields().field(0).number(),
3051+
50000);
3052+
EXPECT_EQ(descriptor_set.file(2).options().unknown_fields().field(0).varint(),
3053+
99);
3054+
}
3055+
3056+
TEST_F(CommandLineInterfaceTest, ImportOption_DescriptorSetIn) {
3057+
FileDescriptorSet file_descriptor_set;
3058+
3059+
DescriptorProto::descriptor()->file()->CopyTo(file_descriptor_set.add_file());
3060+
pb::CppFeatures::descriptor()->file()->CopyTo(file_descriptor_set.add_file());
3061+
3062+
FileDescriptorProto* file = file_descriptor_set.add_file();
3063+
file->set_syntax("editions");
3064+
file->set_edition(Edition::EDITION_2024);
3065+
file->add_option_dependency(pb::CppFeatures::descriptor()->file()->name());
3066+
file->set_name("foo.proto");
3067+
DescriptorProto* message = file->add_message_type();
3068+
message->set_name("Foo");
3069+
FieldDescriptorProto* field = message->add_field();
3070+
field->set_type(FieldDescriptorProto::TYPE_STRING);
3071+
field->set_name("a");
3072+
field->set_number(1);
3073+
field->mutable_options()
3074+
->mutable_features()
3075+
->MutableExtension(pb::cpp)
3076+
->set_string_type(pb::CppFeatures::VIEW);
3077+
3078+
WriteDescriptorSet("foo.bin", &file_descriptor_set);
3079+
Run("protocol_compiler --test_out=$tmpdir "
3080+
"--descriptor_set_out=$tmpdir/descriptor_set "
3081+
"--descriptor_set_in=$tmpdir/foo.bin foo.proto");
3082+
3083+
ExpectNoErrors();
3084+
3085+
FileDescriptorSet descriptor_set;
3086+
ReadDescriptorSet("descriptor_set", &descriptor_set);
3087+
ASSERT_FALSE(HasFatalFailure());
3088+
ASSERT_EQ(descriptor_set.file_size(), 1);
3089+
EXPECT_EQ(descriptor_set.file(0).name(), "foo.proto");
3090+
// Descriptor set should have custom options set.
3091+
EXPECT_EQ(descriptor_set.file(0)
3092+
.message_type(0)
3093+
.field(0)
3094+
.options()
3095+
.features()
3096+
.GetExtension(pb::cpp)
3097+
.string_type(),
3098+
pb::CppFeatures::VIEW);
3099+
}
3100+
3101+
TEST_F(CommandLineInterfaceTest,
3102+
ImportOption_DescriptorSetIn_MissingOptionDependency) {
3103+
FileDescriptorSet file_descriptor_set;
3104+
3105+
FileDescriptorProto* file = file_descriptor_set.add_file();
3106+
file->set_syntax("editions");
3107+
file->set_edition(Edition::EDITION_2024);
3108+
file->add_option_dependency(pb::CppFeatures::descriptor()->file()->name());
3109+
file->set_name("foo.proto");
3110+
DescriptorProto* message = file->add_message_type();
3111+
message->set_name("Foo");
3112+
FieldDescriptorProto* field = message->add_field();
3113+
field->set_type(FieldDescriptorProto::TYPE_STRING);
3114+
field->set_name("a");
3115+
field->set_number(1);
3116+
field->mutable_options()
3117+
->mutable_features()
3118+
->MutableExtension(pb::cpp)
3119+
->set_string_type(pb::CppFeatures::VIEW);
3120+
3121+
WriteDescriptorSet("foo.bin", &file_descriptor_set);
3122+
Run("protocol_compiler --test_out=$tmpdir "
3123+
"--descriptor_set_out=$tmpdir/descriptor_set "
3124+
"--descriptor_set_in=$tmpdir/foo.bin foo.proto");
3125+
3126+
ExpectNoErrors();
3127+
3128+
FileDescriptorSet descriptor_set;
3129+
ReadDescriptorSet("descriptor_set", &descriptor_set);
3130+
ASSERT_FALSE(HasFatalFailure());
3131+
ASSERT_EQ(descriptor_set.file_size(), 1);
3132+
EXPECT_EQ(descriptor_set.file(0).name(), "foo.proto");
3133+
// Descriptor set should have custom options set.
3134+
EXPECT_EQ(descriptor_set.file(0)
3135+
.message_type(0)
3136+
.field(0)
3137+
.options()
3138+
.features()
3139+
.GetExtension(pb::cpp)
3140+
.string_type(),
3141+
pb::CppFeatures::VIEW);
3142+
}
3143+
3144+
TEST_F(CommandLineInterfaceTest, ImportOption_DescriptorSetIn_MissingImport) {
3145+
FileDescriptorSet file_descriptor_set;
3146+
3147+
CreateTempFile("foo.proto",
3148+
R"schema(
3149+
edition = "2024";
3150+
import option "bar.proto";
3151+
import option "google/protobuf/cpp_features.proto";
3152+
option features.(pb.cpp).string_type = VIEW;
3153+
)schema");
3154+
3155+
WriteDescriptorSet("foo.bin", &file_descriptor_set);
3156+
Run("protocol_compiler --test_out=$tmpdir --proto_path=$tmpdir "
3157+
"--descriptor_set_out=$tmpdir/descriptor_set "
3158+
"--descriptor_set_in=$tmpdir/foo.bin foo.proto");
3159+
3160+
ExpectErrorSubstring("bar.proto: File not found");
3161+
ExpectErrorSubstring(
3162+
"google/protobuf/cpp_features.proto: File not found");
3163+
ExpectErrorSubstring("foo.proto:5:14: Option \"features.(pb.cpp)\" unknown");
3164+
}
3165+
29813166
TEST_F(CommandLineInterfaceTest, WriteDescriptorSetWithDuplicates) {
29823167
CreateTempFile("foo.proto",
29833168
"syntax = \"proto2\";\n"
@@ -3250,32 +3435,6 @@ TEST_F(CommandLineInterfaceTest, OptionImportWithDebugRedactDeeplyNested) {
32503435
"option file_opt marked debug_redact");
32513436
}
32523437

3253-
TEST_F(CommandLineInterfaceTest, DisallowMissingOptionImportsDescriptorSetIn) {
3254-
FileDescriptorSet file_descriptor_set;
3255-
3256-
FileDescriptorProto* file = file_descriptor_set.add_file();
3257-
file->set_syntax("editions");
3258-
file->set_edition(Edition::EDITION_2024);
3259-
file->set_name("foo.proto");
3260-
file->add_option_dependency("bar.proto");
3261-
file->add_message_type()->set_name("Foo");
3262-
3263-
// Add an unknown field to the file options to make it look like a custom
3264-
// option.
3265-
file->mutable_message_type(0)
3266-
->mutable_options()
3267-
->mutable_unknown_fields()
3268-
->AddVarint(123, 456);
3269-
3270-
WriteDescriptorSet("foo.bin", &file_descriptor_set);
3271-
3272-
Run("protocol_compiler --descriptor_set_out=$tmpdir/descriptor_set "
3273-
"--include_imports --descriptor_set_in=$tmpdir/foo.bin "
3274-
"--proto_path=$tmpdir --experimental_editions foo.proto");
3275-
3276-
ExpectErrorSubstring("foo.proto: Import \"bar.proto\" was not found");
3277-
}
3278-
32793438
TEST_F(CommandLineInterfaceTest, DescriptorSetOptionRetention) {
32803439
// clang-format off
32813440
CreateTempFile(

0 commit comments

Comments
 (0)