Skip to content

Commit

Permalink
Allow hyphens in Python package names
Browse files Browse the repository at this point in the history
- Push policy decision on hyphens into PythonSemantics
- Revert PyCommon#validatePackageName() to non-static, to match other validation methods. (Static is more important for the initialization functions called from the constructor, to enforce that any init ordering dependencies are explicit.)

Fixes #9171.

RELNOTES: py_binary now tolerates package paths that contain hyphens ('-'). Note that such paths might not be importable from within Python code.
PiperOrigin-RevId: 340484250
  • Loading branch information
brandjon authored and copybara-github committed Nov 3, 2020
1 parent 12224e6 commit 362dbeb
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ public String getSrcsVersionDocURL() {
public void validate(RuleContext ruleContext, PyCommon common) {
}

@Override
public boolean prohibitHyphensInPackagePaths() {
return false;
}

@Override
public void collectRunfilesForBinary(
RuleContext ruleContext, Runfiles.Builder builder, PyCommon common, CcInfo ccInfo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public PyCommon(
this.transitivePythonSources = initTransitivePythonSources(ruleContext);
this.directPythonSources =
initAndMaybeValidateDirectPythonSources(
ruleContext, /*validate=*/ validatePackageAndSources);
ruleContext, semantics, /*validate=*/ validatePackageAndSources);
this.usesSharedLibraries = initUsesSharedLibraries(ruleContext);
this.imports = initImports(ruleContext, semantics);
this.hasPy2OnlySources = initHasPy2OnlySources(ruleContext, this.sourcesVersion);
Expand All @@ -229,7 +229,7 @@ public PyCommon(
validateLegacyProviderNotUsedIfDisabled();
maybeValidateLoadedFromBzl();
if (validatePackageAndSources) {
validatePackageName(ruleContext);
validatePackageName();
}
}

Expand Down Expand Up @@ -282,13 +282,15 @@ private static void collectTransitivePythonSourcesFromDeps(
}

private static List<Artifact> initAndMaybeValidateDirectPythonSources(
RuleContext ruleContext, boolean validate) {
RuleContext ruleContext, PythonSemantics semantics, boolean validate) {
List<Artifact> sourceFiles = new ArrayList<>();
// TODO(bazel-team): Need to get the transitive deps closure, not just the sources of the rule.
for (TransitiveInfoCollection src :
ruleContext.getPrerequisitesIf("srcs", FileProvider.class)) {
// Make sure that none of the sources contain hyphens.
if (validate && Util.containsHyphen(src.getLabel().getPackageFragment())) {
if (validate
&& semantics.prohibitHyphensInPackagePaths()
&& Util.containsHyphen(src.getLabel().getPackageFragment())) {
ruleContext.attributeError(
"srcs", src.getLabel() + ": paths to Python packages may not contain '-'");
}
Expand Down Expand Up @@ -613,8 +615,9 @@ private void maybeValidateLoadedFromBzl() {
}

/** Checks that the package name of this Python rule does not contain a '-'. */
private static void validatePackageName(RuleContext ruleContext) {
if (Util.containsHyphen(ruleContext.getLabel().getPackageFragment())) {
private void validatePackageName() {
if (semantics.prohibitHyphensInPackagePaths()
&& Util.containsHyphen(ruleContext.getLabel().getPackageFragment())) {
ruleContext.ruleError("paths to Python packages may not contain '-'");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ public interface PythonSemantics {
*/
void validate(RuleContext ruleContext, PyCommon common);

/**
* Returns whether we are prohibiting hyphen ('-') characters in the package paths of Python
* targets and source files.
*/
boolean prohibitHyphensInPackagePaths();

/**
* Extends for the default and data runfiles of {@code py_binary} and {@code py_test} rules with
* custom elements.
Expand All @@ -61,14 +67,10 @@ void collectDefaultRunfilesForBinary(
/** Collects a rule's default runfiles. */
void collectDefaultRunfiles(RuleContext ruleContext, Runfiles.Builder builder);

/**
* Returns the coverage instrumentation specification to be used in Python rules.
*/
/** Returns the coverage instrumentation specification to be used in Python rules. */
InstrumentationSpec getCoverageInstrumentationSpec();

/**
* Utility function to compile multiple .py files to .pyc files, if required.
*/
/** Utility function to compile multiple .py files to .pyc files, if required. */
Collection<Artifact> precompiledPythonFiles(
RuleContext ruleContext, Collection<Artifact> sources, PyCommon common);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,33 +436,29 @@ public void explicitInitPy_CanBeSelectivelyEnabled() throws Exception {
}

@Test
public void packageNameCannotHaveHyphen() throws Exception {
checkError(
"pkg-hyphenated",
"foo",
// error:
"paths to Python packages may not contain '-'",
// build file:
public void packageNameCanHaveHyphen() throws Exception {
scratch.file(
"pkg-hyphenated/BUILD", //
"py_binary(",
" name = 'foo',",
" srcs = ['foo.py'],",
")");
assertThat(getConfiguredTarget("//pkg-hyphenated:foo")).isNotNull();
assertNoEvents();
}

@Test
public void srcsPackageNameCannotHaveHyphen() throws Exception {
public void srcsPackageNameCanHaveHyphen() throws Exception {
scratch.file(
"pkg-hyphenated/BUILD", //
"exports_files(['bar.py'])");
checkError(
"otherpkg",
"foo",
// error:
"paths to Python packages may not contain '-'",
// build file:
scratch.file(
"otherpkg/BUILD", //
"py_binary(",
" name = 'foo',",
" srcs = ['foo.py', '//pkg-hyphenated:bar.py'],",
")");
assertThat(getConfiguredTarget("//otherpkg:foo")).isNotNull();
assertNoEvents();
}
}

0 comments on commit 362dbeb

Please sign in to comment.