Skip to content

Commit

Permalink
Add tests for the removal of the check for hyphens in a Python execut…
Browse files Browse the repository at this point in the history 
…able rule's package.

RELNOTES: none
PiperOrigin-RevId: 356602533
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed Feb 9, 2021
1 parent 09ff985 commit 3a0ca0c
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 40 deletions.
9 changes: 4 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,10 @@ private static String getOrderErrorMessage(String fieldName, Order expected, Ord
fieldName, expected.getStarlarkName(), actual.getStarlarkName());
}

// TODO(bazel-team): validatePackageAndSources is the result of refactoring while preserving
// TODO(bazel-team): validateSources is the result of refactoring while preserving
// legacy behavior across some (but not all) Google-internal uses of PyCommon. Ideally all call
// sites should be updated to expect the same validation steps.
public PyCommon(
RuleContext ruleContext, PythonSemantics semantics, boolean validatePackageAndSources) {
public PyCommon(RuleContext ruleContext, PythonSemantics semantics, boolean validateSources) {
this.ruleContext = ruleContext;
this.semantics = semantics;
this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion();
Expand All @@ -193,7 +192,7 @@ public PyCommon(
this.transitivePythonSources = initTransitivePythonSources(ruleContext);
this.directPythonSources =
initAndMaybeValidateDirectPythonSources(
ruleContext, semantics, /*validate=*/ validatePackageAndSources);
ruleContext, semantics, /*validate=*/ validateSources);
this.usesSharedLibraries = initUsesSharedLibraries(ruleContext);
this.imports = initImports(ruleContext, semantics);
this.hasPy2OnlySources = initHasPy2OnlySources(ruleContext, this.sourcesVersion);
Expand Down Expand Up @@ -263,7 +262,7 @@ private static List<Artifact> initAndMaybeValidateDirectPythonSources(
&& semantics.prohibitHyphensInPackagePaths()
&& Util.containsHyphen(src.getLabel().getPackageFragment())) {
ruleContext.attributeError(
"srcs", src.getLabel() + ": paths to Python packages may not contain '-'");
"srcs", src.getLabel() + ": paths to Python sources may not contain '-'");
}
Iterable<Artifact> pySrcs =
FileType.filter(
Expand Down
24 changes: 12 additions & 12 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyExecutable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext));

PythonSemantics semantics = createSemantics();
PyCommon common = new PyCommon(ruleContext, semantics, /*validatePackageAndSources=*/ true);
PyCommon common = new PyCommon(ruleContext, semantics, /*validateSources=*/ true);

List<Artifact> srcs = common.getPythonSources();
List<Artifact> allOutputs =
Expand All @@ -65,20 +65,19 @@ public ConfiguredTarget create(RuleContext ruleContext)

Runfiles commonRunfiles = collectCommonRunfiles(ruleContext, common, semantics, ccInfo);

Runfiles.Builder defaultRunfilesBuilder = new Runfiles.Builder(
ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles())
.merge(commonRunfiles);
Runfiles.Builder defaultRunfilesBuilder =
new Runfiles.Builder(
ruleContext.getWorkspaceName(),
ruleContext.getConfiguration().legacyExternalRunfiles())
.merge(commonRunfiles);
semantics.collectDefaultRunfilesForBinary(ruleContext, common, defaultRunfilesBuilder);

common.createExecutable(ccInfo, defaultRunfilesBuilder);

Runfiles defaultRunfiles = defaultRunfilesBuilder.build();

RunfilesSupport runfilesSupport =
RunfilesSupport.withExecutable(
ruleContext,
defaultRunfiles,
common.getExecutable());
RunfilesSupport.withExecutable(ruleContext, defaultRunfiles, common.getExecutable());

if (ruleContext.hasErrors()) {
return null;
Expand All @@ -102,8 +101,7 @@ public ConfiguredTarget create(RuleContext ruleContext)

RunfilesProvider runfilesProvider = RunfilesProvider.withData(defaultRunfiles, dataRunfiles);

RuleConfiguredTargetBuilder builder =
new RuleConfiguredTargetBuilder(ruleContext);
RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext);
common.addCommonTransitiveInfoProviders(builder, common.getFilesToBuild());

semantics.postInitExecutable(ruleContext, runfilesSupport, common, builder);
Expand Down Expand Up @@ -145,8 +143,10 @@ private static void maybeCreateInitFiles(
private static Runfiles collectCommonRunfiles(
RuleContext ruleContext, PyCommon common, PythonSemantics semantics, CcInfo ccInfo)
throws InterruptedException, RuleErrorException {
Runfiles.Builder builder = new Runfiles.Builder(
ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles());
Runfiles.Builder builder =
new Runfiles.Builder(
ruleContext.getWorkspaceName(),
ruleContext.getConfiguration().legacyExternalRunfiles());
builder.addArtifact(common.getExecutable());
if (common.getConvertedFiles() != null) {
builder.addSymlinks(common.getConvertedFiles());
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public abstract class PyLibrary implements RuleConfiguredTargetFactory {
public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
PythonSemantics semantics = createSemantics();
PyCommon common = new PyCommon(ruleContext, semantics, /*validatePackageAndSources=*/ true);
PyCommon common = new PyCommon(ruleContext, semantics, /*validateSources=*/ true);
semantics.validate(ruleContext, common);

List<Artifact> srcs = common.getPythonSources();
Expand Down
14 changes: 14 additions & 0 deletions ...java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,4 +263,18 @@ public void incompatibleSrcsVersion() throws Exception {
// action to FailAction.
assertNoEvents();
}

@Test
public void targetInPackageWithHyphensOkIfSrcsFromOtherPackage() throws Exception {
scratch.file(
"pkg/BUILD", //
"exports_files(['foo.py'])");
scratch.file(
"pkg-with-hyphens/BUILD",
ruleName + "(",
" name = 'foo',",
" main = '//pkg:foo.py',",
" srcs = ['//pkg:foo.py'])");
getOkPyTarget("//pkg-with-hyphens:foo"); // should not fail
}
}
67 changes: 45 additions & 22 deletions src/test/java/com/google/devtools/build/lib/rules/python/PyLibraryConfiguredTargetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public void versionIs3IfSetByFlag() throws Exception {

@Test
public void filesToBuild() throws Exception {
scratch.file("pkg/BUILD",
scratch.file(
"pkg/BUILD", //
"py_library(",
" name = 'foo',",
" srcs = ['foo.py'])");
Expand All @@ -85,7 +86,9 @@ public void filesToBuild() throws Exception {

@Test
public void srcsCanContainRuleGeneratingPyAndNonpyFiles() throws Exception {
scratchConfiguredTarget("pkg", "foo",
scratchConfiguredTarget(
"pkg",
"foo",
// build file:
"py_binary(",
" name = 'foo',",
Expand All @@ -95,27 +98,32 @@ public void srcsCanContainRuleGeneratingPyAndNonpyFiles() throws Exception {
" outs = ['bar.cc', 'bar.py'],",
" cmd = 'touch $(OUTS)')");
assertNoEvents();
}
}

@Test
public void whatIfSrcsContainsRuleGeneratingNoPyFiles() throws Exception {
// In Bazel it's an error, in Blaze it's a warning.
String[] lines = {
"py_binary(",
" name = 'foo',",
" srcs = ['foo.py', ':bar'])",
"genrule(",
" name = 'bar',",
" outs = ['bar.cc'],",
" cmd = 'touch $(OUTS)')"};
"py_binary(",
" name = 'foo',",
" srcs = ['foo.py', ':bar'])",
"genrule(",
" name = 'bar',",
" outs = ['bar.cc'],",
" cmd = 'touch $(OUTS)')"
};
if (analysisMock.isThisBazel()) {
checkError("pkg", "foo",
checkError(
"pkg",
"foo",
// error:
"'//pkg:bar' does not produce any py_binary srcs files",
// build file:
lines);
} else {
checkWarning("pkg", "foo",
checkWarning(
"pkg",
"foo",
// warning:
"rule '//pkg:bar' does not produce any Python source files",
// build file:
Expand All @@ -125,21 +133,36 @@ public void whatIfSrcsContainsRuleGeneratingNoPyFiles() throws Exception {

@Test
public void filesToCompile() throws Exception {
ConfiguredTarget lib = scratchConfiguredTarget("pkg", "lib",
// build file:
"py_library(name = 'lib', srcs = ['lib.py'], deps = [':bar'])",
"py_library(name = 'bar', srcs = ['bar.py'], deps = [':baz'])",
"py_library(name = 'baz', srcs = ['baz.py'])");
ConfiguredTarget lib =
scratchConfiguredTarget(
"pkg",
"lib",
// build file:
"py_library(name = 'lib', srcs = ['lib.py'], deps = [':bar'])",
"py_library(name = 'bar', srcs = ['bar.py'], deps = [':baz'])",
"py_library(name = 'baz', srcs = ['baz.py'])");

assertThat(
ActionsTestUtil.baseNamesOf(
getOutputGroup(lib, OutputGroupInfo.COMPILATION_PREREQUISITES)))
ActionsTestUtil.baseNamesOf(
getOutputGroup(lib, OutputGroupInfo.COMPILATION_PREREQUISITES)))
.isEqualTo("baz.py bar.py lib.py");

// compilationPrerequisites should be included in filesToCompile.
assertThat(
ActionsTestUtil.baseNamesOf(getOutputGroup(lib, OutputGroupInfo.FILES_TO_COMPILE)))
assertThat(ActionsTestUtil.baseNamesOf(getOutputGroup(lib, OutputGroupInfo.FILES_TO_COMPILE)))
.isEqualTo("baz.py bar.py lib.py");
}
}

@Test
public void libraryTargetCanBeInPackageWithHyphensIfSourcesAreRemote() throws Exception {
scratch.file(
"pkg/BUILD", //
"exports_files(['foo.py'])");
scratchConfiguredTarget(
"pkg-with-hyphens", //
"foo",
"py_library(",
" name = 'foo',",
" srcs = ['//pkg:foo.py'])");
assertNoEvents();
}
}

0 comments on commit 3a0ca0c

Please sign in to comment.