Skip to content

Commit 3efaa32

Browse files
comiuscopybara-github
comius
authored and
copybara-github
committed
Remove incompatible_generated_protos_in_virtual_imports
The flag obscured bugs around generated proto files. Those were corrected in 6c6c196. proto_source_root is now set to path relative to output dir, no matter if the files are generated. This was always the case for a monorepo (consider a proto_library that mixes source file and generated file). The case was just corrected for the multi-repo world. For backwards compatibility paths with _virtual_imports still contain bin or genfiles dir. This will be removed once proto_common.compile automatically computes output directory. See the reasons for introducing the flag in #9215 PiperOrigin-RevId: 536654289 Change-Id: I61379a99ece4c5b0db7c222b1961c7dc9b87b137
1 parent 0a43c70 commit 3efaa32

File tree

6 files changed

+26
-178
lines changed

6 files changed

+26
-178
lines changed

‎src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,15 @@ public static final class AllCommandGraveyardOptions extends OptionsBase {
562562
effectTags = {OptionEffectTag.NO_OP},
563563
help = "No-op.")
564564
public boolean useForkJoinPool;
565+
566+
@Option(
567+
name = "incompatible_generated_protos_in_virtual_imports",
568+
defaultValue = "true",
569+
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
570+
effectTags = {OptionEffectTag.NO_OP},
571+
metadataTags = {OptionMetadataTag.DEPRECATED},
572+
help = "No-op.")
573+
public boolean generatedProtosInVirtualImports;
565574
}
566575

567576
@Override

‎src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -44,16 +44,6 @@ public class ProtoConfiguration extends Fragment implements ProtoConfigurationAp
4444

4545
/** Command line options. */
4646
public static class Options extends FragmentOptions {
47-
@Option(
48-
name = "incompatible_generated_protos_in_virtual_imports",
49-
defaultValue = "true",
50-
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
51-
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
52-
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
53-
help =
54-
"If set, generated .proto files are put into a virtual import directory. For more "
55-
+ "information, see https://github.com/bazelbuild/bazel/issues/9215")
56-
public boolean generatedProtosInVirtualImports;
5747

5848
@Option(
5949
name = "protocopt",
@@ -196,7 +186,6 @@ public FragmentOptions getExec() {
196186
exec.strictPublicImports = strictPublicImports;
197187
exec.ccProtoLibraryHeaderSuffixes = ccProtoLibraryHeaderSuffixes;
198188
exec.ccProtoLibrarySourceSuffixes = ccProtoLibrarySourceSuffixes;
199-
exec.generatedProtosInVirtualImports = generatedProtosInVirtualImports;
200189
return exec;
201190
}
202191
}
@@ -329,19 +318,4 @@ public List<String> ccProtoLibrarySourceSuffixesForStarlark(StarlarkThread threa
329318
public List<String> ccProtoLibrarySourceSuffixes() {
330319
return ccProtoLibrarySourceSuffixes;
331320
}
332-
333-
334-
@StarlarkMethod(
335-
name = "generated_protos_in_virtual_imports",
336-
useStarlarkThread = true,
337-
documented = false)
338-
public boolean generatedProtosInVirtualImportsForStarlark(StarlarkThread thread)
339-
throws EvalException {
340-
ProtoCommon.checkPrivateStarlarkificationAllowlist(thread);
341-
return generatedProtosInVirtualImports();
342-
}
343-
344-
public boolean generatedProtosInVirtualImports() {
345-
return options.generatedProtosInVirtualImports;
346-
}
347321
}

‎src/main/starlark/builtins_bzl/common/proto/proto_library.bzl‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,7 @@ def _process_srcs(ctx, srcs, import_prefix, strip_import_prefix):
118118
Returns:
119119
(str, [File]) A pair of proto_path and virtual_sources.
120120
"""
121-
generate_protos_in_virtual_imports = False
122-
if ctx.fragments.proto.generated_protos_in_virtual_imports():
123-
generate_protos_in_virtual_imports = any([not src.is_source for src in srcs])
124-
125-
if import_prefix != "" or strip_import_prefix != "" or generate_protos_in_virtual_imports:
121+
if import_prefix != "" or strip_import_prefix != "":
126122
# Use virtual source roots
127123
return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix)
128124
else:

‎src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,6 @@ public void generateCode_protocOpts() throws Exception {
388388
*/
389389
@Test
390390
public void generateCode_directGeneratedProtos() throws Exception {
391-
useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
392391
scratch.file(
393392
"bar/BUILD",
394393
TestConstants.LOAD_PROTO_LIBRARY,
@@ -419,7 +418,6 @@ public void generateCode_directGeneratedProtos() throws Exception {
419418
*/
420419
@Test
421420
public void generateCode_inDirectGeneratedProtos() throws Exception {
422-
useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
423421
scratch.file(
424422
"bar/BUILD",
425423
TestConstants.LOAD_PROTO_LIBRARY,
@@ -450,34 +448,18 @@ public void generateCode_inDirectGeneratedProtos() throws Exception {
450448
*/
451449
@Test
452450
@TestParameters({
453-
"{virtual: false, sibling: false, generated: false, expectedFlags:"
451+
"{sibling: false, generated: false, expectedFlags:"
454452
+ " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
455-
"{virtual: false, sibling: false, generated: true, expectedFlags:"
453+
"{sibling: false, generated: true, expectedFlags:"
456454
+ " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo',"
457455
+ " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/E.proto']}",
458-
"{virtual: true, sibling: false, generated: false,expectedFlags:"
459-
+ " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
460-
"{virtual: true, sibling: false, generated: true, expectedFlags:"
461-
+ " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e',"
462-
+ " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e/e/E.proto']}",
463-
"{virtual: true, sibling: true, generated: false,expectedFlags:"
464-
+ " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
465-
"{virtual: true, sibling: true, generated: true, expectedFlags:"
466-
+ " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e',"
467-
+ " '-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e/e/E.proto']}",
468-
"{virtual: false, sibling: true, generated: false,expectedFlags:"
456+
"{sibling: true, generated: false,expectedFlags:"
469457
+ " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
470-
"{virtual: false, sibling: true, generated: true, expectedFlags:"
458+
"{sibling: true, generated: true, expectedFlags:"
471459
+ " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin','-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/E.proto']}",
472460
})
473461
public void generateCode_externalProtoLibrary(
474-
boolean virtual, boolean sibling, boolean generated, List<String> expectedFlags)
475-
throws Exception {
476-
if (virtual) {
477-
useConfiguration("--incompatible_generated_protos_in_virtual_imports");
478-
} else {
479-
useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
480-
}
462+
boolean sibling, boolean generated, List<String> expectedFlags) throws Exception {
481463
if (sibling) {
482464
setBuildLanguageOptions("--experimental_sibling_repository_layout");
483465
}

‎src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java‎

Lines changed: 5 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -412,8 +412,7 @@ public void testStripImportPrefixWithDeps() throws Exception {
412412
".");
413413
}
414414

415-
private void testExternalRepoWithGeneratedProto(
416-
boolean siblingRepoLayout, boolean useVirtualImports) throws Exception {
415+
private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throws Exception {
417416
if (!isThisBazel()) {
418417
return;
419418
}
@@ -423,9 +422,6 @@ private void testExternalRepoWithGeneratedProto(
423422
if (siblingRepoLayout) {
424423
setBuildLanguageOptions("--experimental_sibling_repository_layout");
425424
}
426-
if (!useVirtualImports) {
427-
useConfiguration("--noincompatible_generated_protos_in_virtual_imports");
428-
}
429425
invalidatePackages();
430426

431427
scratch.file("/foo/WORKSPACE");
@@ -445,12 +441,7 @@ private void testExternalRepoWithGeneratedProto(
445441
siblingRepoLayout ? RepositoryName.create("foo") : RepositoryName.MAIN)
446442
.toString();
447443
String fooProtoRoot;
448-
if (useVirtualImports) {
449-
fooProtoRoot =
450-
genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x";
451-
} else {
452-
fooProtoRoot = (siblingRepoLayout ? genfiles : genfiles + "/external/foo");
453-
}
444+
fooProtoRoot = (siblingRepoLayout ? genfiles : genfiles + "/external/foo");
454445
ConfiguredTarget a = getConfiguredTarget("//a:a");
455446
ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);
456447
assertThat(aInfo.getTransitiveProtoSourceRoots().toList()).containsExactly(".", fooProtoRoot);
@@ -462,24 +453,12 @@ private void testExternalRepoWithGeneratedProto(
462453

463454
@Test
464455
public void testExternalRepoWithGeneratedProto_withSubdirRepoLayout() throws Exception {
465-
testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, true);
456+
testExternalRepoWithGeneratedProto(/* siblingRepoLayout= */ false);
466457
}
467458

468459
@Test
469460
public void test_siblingRepoLayout_externalRepoWithGeneratedProto() throws Exception {
470-
testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, true);
471-
}
472-
473-
@Test
474-
public void testExternalRepoWithGeneratedProto_withSubdirRepoLayoutAndNoVritualImports()
475-
throws Exception {
476-
testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, false);
477-
}
478-
479-
@Test
480-
public void test_siblingRepoLayout_externalRepoWithGeneratedProtoAndNoVritualImports()
481-
throws Exception {
482-
testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, false);
461+
testExternalRepoWithGeneratedProto(/* siblingRepoLayout= */ true);
483462
}
484463

485464
@Test
@@ -1076,35 +1055,7 @@ public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Except
10761055
}
10771056

10781057
@Test
1079-
public void testSourceAndGeneratedProtoFiles_Bazel() throws Exception {
1080-
if (!isThisBazel()) {
1081-
return;
1082-
}
1083-
1084-
scratch.file(
1085-
"a/BUILD",
1086-
TestConstants.LOAD_PROTO_LIBRARY,
1087-
"genrule(name='g', srcs=[], outs=['g.proto'], cmd = '')",
1088-
"proto_library(name='p', srcs=['s.proto', 'g.proto'])");
1089-
1090-
ImmutableList<String> commandLine =
1091-
allArgsForAction((SpawnAction) getDescriptorWriteAction("//a:p"));
1092-
String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
1093-
assertThat(commandLine)
1094-
.containsAtLeast(
1095-
"-Ia/s.proto=" + genfiles + "/a/_virtual_imports/p/a/s.proto",
1096-
"-Ia/g.proto=" + genfiles + "/a/_virtual_imports/p/a/g.proto");
1097-
}
1098-
1099-
@Test
1100-
public void testSourceAndGeneratedProtoFiles_Blaze() throws Exception {
1101-
if (!isThisBazel()) {
1102-
return;
1103-
}
1104-
1105-
// Simulate behavoiur of Blaze's `proto_library` in Bazel.
1106-
useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
1107-
1058+
public void testSourceAndGeneratedProtoFiles() throws Exception {
11081059
scratch.file(
11091060
"a/BUILD",
11101061
TestConstants.LOAD_PROTO_LIBRARY,
@@ -1210,43 +1161,12 @@ public void testProtoLibraryWithVirtualProtoSourceRoot() throws Exception {
12101161
.containsExactly("foo/x/a.proto");
12111162
}
12121163

1213-
@Test
1214-
public void testProtoLibraryWithGeneratedSources_Bazel() throws Exception {
1215-
if (!isThisBazel()) {
1216-
return;
1217-
}
1218-
1219-
useConfiguration("--incompatible_generated_protos_in_virtual_imports=true");
1220-
1221-
scratch.file(
1222-
"x/BUILD",
1223-
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
1224-
"proto_library(name='foo', srcs=['generated.proto'])");
1225-
1226-
String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
1227-
ProtoInfo provider = getConfiguredTarget("//x:foo").get(ProtoInfo.PROVIDER);
1228-
assertThat(
1229-
Iterables.transform(
1230-
provider.getDirectSources(), s -> s.getSourceFile().getExecPath().getPathString()))
1231-
.containsExactly(genfiles + "/x/_virtual_imports/foo/x/generated.proto");
1232-
assertThat(
1233-
Iterables.transform(
1234-
provider.getDirectSources(), s -> s.getSourceRoot().getSafePathString()))
1235-
.containsExactly("x/_virtual_imports/foo");
1236-
assertThat(
1237-
Iterables.transform(
1238-
provider.getDirectSources(), s -> s.getImportPath().getPathString()))
1239-
.containsExactly("x/generated.proto");
1240-
}
1241-
12421164
@Test
12431165
public void testProtoLibraryWithGeneratedSources_Blaze() throws Exception {
12441166
if (!isThisBazel()) {
12451167
return;
12461168
}
12471169

1248-
useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
1249-
12501170
scratch.file(
12511171
"x/BUILD",
12521172
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
@@ -1268,45 +1188,12 @@ public void testProtoLibraryWithGeneratedSources_Blaze() throws Exception {
12681188
.containsExactly("x/generated.proto");
12691189
}
12701190

1271-
@Test
1272-
public void testProtoLibraryWithMixedSources_Bazel() throws Exception {
1273-
if (!isThisBazel()) {
1274-
return;
1275-
}
1276-
1277-
useConfiguration("--incompatible_generated_protos_in_virtual_imports=true");
1278-
1279-
scratch.file(
1280-
"x/BUILD",
1281-
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",
1282-
"proto_library(name='foo', srcs=['generated.proto', 'a.proto'])");
1283-
1284-
String genfiles = getTargetConfiguration().getGenfilesFragment(RepositoryName.MAIN).toString();
1285-
ProtoInfo provider = getConfiguredTarget("//x:foo").get(ProtoInfo.PROVIDER);
1286-
assertThat(
1287-
Iterables.transform(
1288-
provider.getDirectSources(), s -> s.getSourceFile().getExecPath().getPathString()))
1289-
.containsExactly(
1290-
genfiles + "/x/_virtual_imports/foo/x/generated.proto",
1291-
genfiles + "/x/_virtual_imports/foo/x/a.proto");
1292-
assertThat(
1293-
Iterables.transform(
1294-
provider.getDirectSources(), s -> s.getSourceRoot().getSafePathString()))
1295-
.containsExactly("x/_virtual_imports/foo", "x/_virtual_imports/foo");
1296-
assertThat(
1297-
Iterables.transform(
1298-
provider.getDirectSources(), s -> s.getImportPath().getPathString()))
1299-
.containsExactly("x/generated.proto", "x/a.proto");
1300-
}
1301-
13021191
@Test
13031192
public void testProtoLibraryWithMixedSources_Blaze() throws Exception {
13041193
if (!isThisBazel()) {
13051194
return;
13061195
}
13071196

1308-
useConfiguration("--incompatible_generated_protos_in_virtual_imports=false");
1309-
13101197
scratch.file(
13111198
"x/BUILD",
13121199
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')",

‎src/test/shell/bazel/bazel_proto_library_test.sh‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -813,13 +813,13 @@ message H {
813813
}
814814
EOF
815815

816-
bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h >& $TEST_log || fail "failed"
817-
bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto >& $TEST_log || fail "failed"
818-
bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_java_proto >& $TEST_log || fail "failed"
816+
bazel build -s --noexperimental_sibling_repository_layout //h >& $TEST_log || fail "failed"
817+
bazel build -s --noexperimental_sibling_repository_layout //h:h_cc_proto >& $TEST_log || fail "failed"
818+
bazel build -s --noexperimental_sibling_repository_layout //h:h_java_proto >& $TEST_log || fail "failed"
819819

820-
bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h -s >& $TEST_log || fail "failed"
821-
bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto -s >& $TEST_log || fail "failed"
822-
bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_java_proto -s >& $TEST_log || fail "failed"
820+
bazel build -s --experimental_sibling_repository_layout //h -s >& $TEST_log || fail "failed"
821+
bazel build -s --experimental_sibling_repository_layout //h:h_cc_proto -s >& $TEST_log || fail "failed"
822+
bazel build -s --experimental_sibling_repository_layout //h:h_java_proto -s >& $TEST_log || fail "failed"
823823

824824
expect_not_log "warning: directory does not exist." # --proto_path is wrong
825825

0 commit comments

Comments
 (0)