Skip to content

Commit

Permalink
Flip the flag --incompatible_static_name_resolution_in_build_files
Browse files Browse the repository at this point in the history
    Fixes bazelbuild/bazel#8022

    RELNOTES: --incompatible_static_name_resolution_in_build_files is now enabled by default
    PiperOrigin-RevId: 249521083
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 633d00a commit 4c4fbd0
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 232 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,21 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
+ "instead return a list of provider instances.")
public boolean incompatibleDisallowStructProviderSyntax;

@Option(
name = "incompatible_disallow_old_octal_notation",
defaultValue = "true",
category = "incompatible changes",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If set to true, octal numbers like `0123` are forbidden, they should be written "
+ "`0o123` instead. See https://github.com/bazelbuild/bazel/issues/8059")
public boolean incompatibleDisallowOldOctalNotation;

/** Controls legacy arguments to ctx.actions.Args#add. */
@Option(
name = "incompatible_disallow_old_style_args_add",
Expand Down Expand Up @@ -619,6 +634,7 @@ public StarlarkSemantics toSkylarkSemantics() {
.incompatibleDisallowLoadLabelsToCrossPackageBoundaries(
incompatibleDisallowLoadLabelsToCrossPackageBoundaries)
.incompatibleDisallowNativeInBuildFile(incompatibleDisallowNativeInBuildFile)
.incompatibleDisallowOldOctalNotation(incompatibleDisallowOldOctalNotation)
.incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
.incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

public abstract boolean incompatibleDisallowNativeInBuildFile();

public abstract boolean incompatibleDisallowOldOctalNotation();

public abstract boolean incompatibleDisallowOldStyleArgsAdd();

public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed();
Expand Down Expand Up @@ -232,6 +234,7 @@ public static Builder builderWithDefaults() {
.incompatibleDisallowLegacyJavaInfo(false)
.incompatibleDisallowLoadLabelsToCrossPackageBoundaries(true)
.incompatibleDisallowNativeInBuildFile(true)
.incompatibleDisallowOldOctalNotation(true)
.incompatibleDisallowOldStyleArgsAdd(true)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false)
.incompatibleDisallowStructProviderSyntax(false)
Expand Down Expand Up @@ -297,6 +300,8 @@ public abstract static class Builder {

public abstract Builder incompatibleDisallowLoadLabelsToCrossPackageBoundaries(boolean value);

public abstract Builder incompatibleDisallowOldOctalNotation(boolean value);

public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value);

public abstract Builder incompatibleDisallowNativeInBuildFile(boolean value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,24 +166,6 @@ public void testGeneratedArtifact() throws Exception {
assertThat(action.getClass()).isSameInstanceAs(FailAction.class);
}

@Test
public void testGetArtifactOwnerInStarlark() throws Exception {
scratch.file(
"foo/rule.bzl",
"def _impl(ctx):",
" f = ctx.actions.declare_file('rule_output')",
" print('f owner is ' + str(f.owner))",
" ctx.actions.write(",
" output = f,",
" content = 'foo',",
" )",
"gen = rule(implementation = _impl)");
scratch.file("foo/BUILD", "load(':rule.bzl', 'gen')", "gen(name = 'a')");

update("//foo:a");
assertContainsEvent("DEBUG /workspace/foo/rule.bzl:3:3: f owner is //foo:a");
}

@Test
public void testSyntaxErrorInDepPackage() throws Exception {
// Check that a loading error in a dependency is properly reported.
Expand Down Expand Up @@ -466,7 +448,7 @@ public void testOutputFilterWithDebug() throws Exception {
"java/b/rules.bzl",
"def _impl(ctx):",
" print('debug in b')",
" ctx.actions.write(",
" ctx.file_action(",
" output = ctx.outputs.my_output,",
" content = 'foo',",
" )",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,23 @@

import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
import com.google.devtools.build.lib.analysis.BuildView.AnalysisResult;
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventCollector;
import com.google.devtools.build.lib.events.OutputFilter.RegexOutputFilter;
import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
import com.google.devtools.build.lib.query2.output.OutputFormatter;
import com.google.devtools.build.lib.rules.genquery.GenQuery;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Injected;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.DeterministicHelper;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
import com.google.devtools.build.skyframe.NotifyingInMemoryGraph;

import com.google.devtools.build.skyframe.NotifyingHelper.Listener;
import java.util.ArrayList;
import java.util.List;
import java.util.regex.Pattern;
Expand All @@ -62,15 +55,6 @@ protected static int getFrequencyOfErrorsWithLocation(
return frequency;
}

@Override
protected ImmutableList<Injected> getPrecomputedValues() {
ImmutableList.Builder<Injected> result = ImmutableList.builder();
result.addAll(super.getPrecomputedValues());
result.add(PrecomputedValue.injected(
GenQuery.QUERY_OUTPUT_FORMATTERS, OutputFormatter.getDefaultFormatters()));
return result.build();
}

protected final void setupDummyRule() throws Exception {
scratch.file("pkg/BUILD",
"testing_dummy_rule(name='foo', ",
Expand Down Expand Up @@ -105,43 +89,54 @@ protected void runTestDepOnGoodTargetInBadPkgAndTransitiveCycle(boolean incremen
Path symlinkcycleBuildFile = scratch.file("symlinkcycle/BUILD",
"sh_library(name = 'cycle', srcs = glob(['*.sh']))");
Path dirPath = symlinkcycleBuildFile.getParentDirectory();
dirPath.getRelative("foo.sh").createSymbolicLink(new PathFragment("foo.sh"));
dirPath.getRelative("foo.sh").createSymbolicLink(PathFragment.create("foo.sh"));
scratch.file("okaypkg/BUILD",
"sh_library(name = 'transitively-a-cycle',",
" srcs = ['//symlinkcycle:cycle'])");
Path badpkgBuildFile = scratch.file("badpkg/BUILD",
"exports_files(['okay-target'])",
"invalidbuildsyntax");
Path badpkgBuildFile = scratch.file("badpkg/BUILD", "exports_files(['okay-target'])", "fail()");
if (incremental) {
update(defaultFlags().with(Flag.KEEP_GOING), "//okaypkg:transitively-a-cycle");
assertContainsEvent("circular symlinks detected");
eventCollector.clear();
}
update(defaultFlags().with(Flag.KEEP_GOING), "//parent:foo");
assertEquals(1, getFrequencyOfErrorsWithLocation(badpkgBuildFile.asFragment(), eventCollector));
assertThat(getFrequencyOfErrorsWithLocation(badpkgBuildFile.asFragment(), eventCollector))
.isEqualTo(1);
// TODO(nharmata): This test currently only works because each BuildViewTest#update call
// dirties all FileNodes that are in error. There is actually a skyframe bug with cycle
// reporting on incremental builds (see b/14622820).
assertContainsEvent("circular symlinks detected");
}

protected void setGraphForTesting(NotifyingInMemoryGraph notifyingInMemoryGraph) {
protected void injectGraphListenerForTesting(Listener listener, boolean deterministic) {
InMemoryMemoizingEvaluator memoizingEvaluator =
(InMemoryMemoizingEvaluator) skyframeExecutor.getEvaluatorForTesting();
memoizingEvaluator.setGraphForTesting(notifyingInMemoryGraph);
memoizingEvaluator.injectGraphTransformerForTesting(
DeterministicHelper.makeTransformer(listener, deterministic));
}

protected void runTestForMultiCpuAnalysisFailure(String badCpu, String goodCpu) throws Exception {
reporter.removeHandler(failFastHandler);
useConfiguration("--experimental_multi_cpu=" + badCpu + "," + goodCpu);
scratch.file("multi/BUILD",
"cc_library(name='cpu', abi='$(TARGET_CPU)', abi_deps={'" + badCpu + "':[':fail']})",
"genrule(name='fail', outs=['file1', 'file2'], executable = 1, cmd='touch $@')");
"config_setting(",
" name = 'config',",
" values = {'cpu': '" + badCpu + "'})",
"cc_library(",
" name = 'cpu',",
" deps = select({",
" ':config': [':fail'],",
" '//conditions:default': []}))",
"genrule(",
" name = 'fail',",
" outs = ['file1', 'file2'],",
" executable = 1,",
" cmd = 'touch $@')");
update(defaultFlags().with(Flag.KEEP_GOING), "//multi:cpu");
AnalysisResult result = getAnalysisResult();
assertThat(result.getTargetsToBuild()).hasSize(1);
ConfiguredTarget targetA = Iterables.get(result.getTargetsToBuild(), 0);
assertEquals(goodCpu, targetA.getConfiguration().getCpu());
assertThat(getConfiguration(targetA).getCpu()).isEqualTo(goodCpu);
// Unfortunately, we get the same error twice - we can't distinguish the configurations.
assertContainsEvent("if genrules produce executables, they are allowed only one output");
}
Expand All @@ -164,9 +159,9 @@ public void analysisFailure(AnalysisFailureEvent event) {
public static class LoadingFailureRecorder {
@Subscribe
public void loadingFailure(LoadingFailureEvent event) {
events.add(Pair.of(event.getFailedTarget(), event.getFailureReason()));
events.add(event);
}

public final List<Pair<Label, Label>> events = new ArrayList<>();
public final List<LoadingFailureEvent> events = new ArrayList<>();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ public void testInsufficientArgumentGlobErrors() throws Exception {
"glob()",
"insufficient arguments received by glob(include: sequence of strings, "
+ "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
+ "allow_empty: bool = <unbound>) (got 0, expected at least 1)");
+ "allow_empty: bool = True) (got 0, expected at least 1)");
}

@Test
Expand All @@ -671,7 +671,7 @@ public void testGlobUnamedExclude() throws Exception {
"glob(['a'], ['b'])",
"too many (2) positional arguments in call to glob(include: sequence of strings, "
+ "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
+ "allow_empty: bool = <unbound>)");
+ "allow_empty: bool = True)");
}

@Test
Expand All @@ -681,7 +681,7 @@ public void testTooManyArgumentsGlobErrors() throws Exception {
"glob(1,2,3,4)",
"too many (4) positional arguments in call to glob(include: sequence of strings, "
+ "*, exclude: sequence of strings = [], exclude_directories: int = 1, "
+ "allow_empty: bool = <unbound>)");
+ "allow_empty: bool = True)");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import static org.junit.Assert.fail;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
Expand All @@ -33,7 +32,7 @@
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.Pair;
Expand Down Expand Up @@ -114,7 +113,6 @@ protected static void assertEvaluates(
new GlobCache(
pkg.getFilename().asPath().getParentDirectory(),
pkg.getPackageIdentifier(),
ImmutableSet.of(),
PackageFactoryApparatus.createEmptyLocator(),
null,
TestUtils.getPool(),
Expand Down Expand Up @@ -206,7 +204,7 @@ protected void assertGlobMatches(
includes,
excludes,
excludeDirs,
Starlark.format("(result == sorted(%r)) or fail('incorrect glob result')", result));
Printer.format("(result == sorted(%r)) or fail('incorrect glob result')", result));

Package pkg = evaluated.first;
GlobCache globCache = evaluated.second;
Expand Down Expand Up @@ -238,7 +236,7 @@ private Pair<Package, GlobCache> evaluateGlob(
Path file =
scratch.file(
"/globs/BUILD",
Starlark.format(
Printer.format(
"result = glob(%r, exclude=%r, exclude_directories=%r)",
includes, excludes, excludeDirs ? 1 : 0),
resultAssertion);
Expand Down
Loading

0 comments on commit 4c4fbd0

Please sign in to comment.