Skip to content

Commit

Permalink
Rollback of commit 5c548f1.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Fixed depot issues.

*** Original change description ***

Automated [] rollback of commit 2e5ec0f.

*** Reason for rollback ***

Breaks some targets in the nightly: see []

*** Original change description ***

Make java_proto_library's strict_deps default to true.
Remove package-level attribute to set the default of strict_deps.
Change the semantics to --strict_deps_java_protos to mean force strict deps of all Java protos to be true regardless of their strict_deps attribute.

--
MOS_MIGRATED_REVID=133981754
  • Loading branch information
cgrushko authored and laszlocsomor committed Sep 23, 2016
1 parent b4b7461 commit 3621ab6
Show file tree
Hide file tree
Showing 7 changed files with 12 additions and 128 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
.allowedRuleClasses("proto_library")
.allowedFileTypes()
.aspect(javaProtoAspect, ASPECT_PARAMETERS))
/* <!-- #BLAZE_RULE(java_lite_proto_library).ATTRIBUTE(deps) -->
/* <!-- #BLAZE_RULE(java_lite_proto_library).ATTRIBUTE(strict_deps) -->
When True, this rule only exposes the protos that it wraps directly. Depending on indirect
protos will break the build and print an 'add_dep' command to correct the build.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("strict_deps", BOOLEAN))
.add(attr("strict_deps", BOOLEAN).value(true))
.add(
attr(LITE_PROTO_RUNTIME_ATTR, LABEL)
.legacyAllowAnyFileType()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
.allowedRuleClasses("proto_library")
.allowedFileTypes()
.aspect(javaProtoAspect, ASPECT_PARAMETERS))
.add(attr("strict_deps", BOOLEAN).undocumented("for migration"))
.add(attr("strict_deps", BOOLEAN).value(true).undocumented("for migration"))
.build();
}

Expand Down
17 changes: 0 additions & 17 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ protected NameConflictException(String message) {
*/
private String defaultHdrsCheck;

/** See getter. */
private TriState defaultStrictDepsJavaProtos = TriState.AUTO;

/** Default copts for cc_* rules. The rules' individual copts will append to this value. */
private ImmutableList<String> defaultCopts;

Expand Down Expand Up @@ -587,15 +584,6 @@ public String getDefaultDeprecation() {
return defaultDeprecation;
}

/**
* Default for 'strict_deps' of Java proto rules, when they aren't explicitly specified.
*
* <p>A value of AUTO is returned when the package didn't itself explicitly specify this value.
*/
public TriState getDefaultStrictDepsJavaProtos() {
return defaultStrictDepsJavaProtos;
}

/** Gets the default header checking mode. */
public String getDefaultHdrsCheck() {
return defaultHdrsCheck != null ? defaultHdrsCheck : "strict";
Expand Down Expand Up @@ -912,11 +900,6 @@ public Builder setDefaultHdrsCheck(String hdrsCheck) {
return this;
}

public Builder setDefaultStrictDepsJavaProtos(TriState value) {
pkg.defaultStrictDepsJavaProtos = value;
return this;
}

/** Sets the default value of copts. Rule-level copts will append to this. */
public Builder setDefaultCopts(List<String> defaultCopts) {
this.defaultCopts = defaultCopts;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,24 +262,6 @@ protected void process(Package.Builder pkgBuilder, Location location,
}
}

/**
* Declares the package() attribute specifying the default value for
* java_proto_library.strict_deps.
*
* <p>This attribute should be considered as undocumented/experimental, and is subject to change
* at any time without prior notice.
*/
private static class DefaultStrictDepsJavaProtos extends PackageArgument<Boolean> {
private DefaultStrictDepsJavaProtos() {
super("default_strict_deps_java_protos", Type.BOOLEAN);
}

@Override
protected void process(Package.Builder pkgBuilder, Location location, Boolean value) {
pkgBuilder.setDefaultStrictDepsJavaProtos(value ? TriState.YES : TriState.NO);
}
}

public static final String PKG_CONTEXT = "$pkg_context";

// Used outside of Bazel!
Expand Down Expand Up @@ -481,7 +463,6 @@ private ImmutableMap<String, PackageArgument<?>> createPackageArguments() {
ImmutableList.Builder<PackageArgument<?>> arguments =
ImmutableList.<PackageArgument<?>>builder()
.add(new DefaultDeprecation())
.add(new DefaultStrictDepsJavaProtos())
.add(new DefaultDistribs())
.add(new DefaultLicenses())
.add(new DefaultTestOnly())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,14 +352,13 @@ public JavaOptimizationModeConverter() {

@Option(
name = "strict_deps_java_protos",
defaultValue = "true",
defaultValue = "false",
category = "undocumented",
help =
"When 'strict-deps' is on, .java files that depend on classes not declared in their rule's "
+ "'deps' fail to build. In other words, it's forbidden to depend on classes obtained "
+ "transitively. This flag controls the behavior of Java proto rules when their "
+ "'strict_deps' attribute is unspecified, and its containing package doesn't specify "
+ "'default_strict_deps_java_protos'."
+ "transitively. When true, Java protos are strict regardless of their 'strict_deps' "
+ "attribute."
)
public boolean strictDepsJavaProtos;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
import static com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType.BOTH;

import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.packages.AttributeContainer;
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.rules.java.JavaCompilationArgs;
import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider;
import com.google.devtools.build.lib.rules.java.JavaConfiguration;
Expand All @@ -28,26 +26,13 @@ public class StrictDepsUtils {
/**
* Returns true iff 'ruleContext' should enforce strict-deps.
*
* <ol>
* <li>If the rule explicitly specifies the 'strict_deps' attribute, returns its value.
* <li>Otherwise, if the package explicitly specifies 'default_strict_deps_java_proto_library',
* returns that value.
* <li>Otherwise, returns the value of the --strict_deps_java_proto_library flag.
* </ol>
*
* Using this method requires requesting the JavaConfiguration fragment.
* <p>Using this method requires requesting the JavaConfiguration fragment.
*/
public static boolean isStrictDepsJavaProtoLibrary(RuleContext ruleContext) {
AttributeContainer attributeContainer = ruleContext.getRule().getAttributeContainer();
if (attributeContainer.isAttributeValueExplicitlySpecified("strict_deps")) {
return (boolean) attributeContainer.getAttr("strict_deps");
}
TriState defaultJavaProtoLibraryStrictDeps =
ruleContext.getRule().getPackage().getDefaultStrictDepsJavaProtos();
if (defaultJavaProtoLibraryStrictDeps == TriState.AUTO) {
return ruleContext.getFragment(JavaConfiguration.class).strictDepsJavaProtos();
if (ruleContext.getFragment(JavaConfiguration.class).strictDepsJavaProtos()) {
return true;
}
return defaultJavaProtoLibraryStrictDeps == TriState.YES;
return (boolean) ruleContext.getRule().getAttributeContainer().getAttr("strict_deps");
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,43 +35,11 @@ public void isStrictDepsJavaProtoLibrary_flagIsFalse_noPackageLevelAttribute() t
"java_proto_library(name = 'b', strict_deps = 0)",
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:a"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:a"))).isTrue();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:c"))).isTrue();
}

@Test
public void isStrictDepsJavaProtoLibrary_flagIsFalse_packageLevelIs0() throws Exception {
useConfiguration("--strict_deps_java_protos=false");

scratch.file(
"y/BUILD",
"package(default_strict_deps_java_protos = 0)",
"java_proto_library(name = 'a')",
"java_proto_library(name = 'b', strict_deps = 0)",
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:a"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:c"))).isTrue();
}

@Test
public void isStrictDepsJavaProtoLibrary_flagIsFalse_packageLevelIs1() throws Exception {
useConfiguration("--strict_deps_java_protos=false");

scratch.file(
"z/BUILD",
"package(default_strict_deps_java_protos = 1)",
"java_proto_library(name = 'a')",
"java_proto_library(name = 'b', strict_deps = 0)",
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:a"))).isTrue();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:c"))).isTrue();
}

@Test
public void isStrictDepsJavaProtoLibrary_flagIsTrue_noPackageLevelAttribute() throws Exception {
useConfiguration("--strict_deps_java_protos=true");
Expand All @@ -83,42 +51,10 @@ public void isStrictDepsJavaProtoLibrary_flagIsTrue_noPackageLevelAttribute() th
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:a"))).isTrue();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:b"))).isTrue();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//x:c"))).isTrue();
}

@Test
public void isStrictDepsJavaProtoLibrary_flagIsTrue_packageLevelIs0() throws Exception {
useConfiguration("--strict_deps_java_protos=true");

scratch.file(
"y/BUILD",
"package(default_strict_deps_java_protos = 0)",
"java_proto_library(name = 'a')",
"java_proto_library(name = 'b', strict_deps = 0)",
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:a"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//y:c"))).isTrue();
}

@Test
public void isStrictDepsJavaProtoLibrary_flagIsTrue_packageLevelIs1() throws Exception {
useConfiguration("--strict_deps_java_protos=true");

scratch.file(
"z/BUILD",
"package(default_strict_deps_java_protos = 1)",
"java_proto_library(name = 'a')",
"java_proto_library(name = 'b', strict_deps = 0)",
"java_proto_library(name = 'c', strict_deps = 1)");

assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:a"))).isTrue();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:b"))).isFalse();
assertThat(StrictDepsUtils.isStrictDepsJavaProtoLibrary(getRuleContext("//z:c"))).isTrue();
}

private RuleContext getRuleContext(String label) throws Exception {
return getRuleContext(getConfiguredTarget(label));
}
Expand Down

0 comments on commit 3621ab6

Please sign in to comment.