Skip to content

Commit

Permalink
Add 'incompatible_default_to_explicit_init_py' flag
Browse files Browse the repository at this point in the history
    This effectively changes the default value of legacy_create_init to false.

    Technically, the attribute type has changed from boolean (default true) to tristate (default auto). Therefore, it is possible to see a semantic change in code that introspects a py_binary's or py_test's attrs dict (via native.existing_rules). We think it is unlikely this will break anyone, and in any case it is not currently possible to gate a rule's definition on an incompatible change flag.

    Closes #9271. Work toward #7386 and #10076.

    RELNOTES: [Python] Added flag --incomaptible_default_to_explicit_init_py to switch the default value of legacy_create_init to True. With this flag enabled, your py_binary and py_test targets will no longer behave as if empty __init__.py files were implicitly littered in your runfiles tree. See [#10076](bazelbuild/bazel#10076).
    PiperOrigin-RevId: 276526057
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 44eb0cd commit d1cedc0
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ The list of source (<code>.py</code>) files that are processed to create the tar
These are created in every directory containing Python source code or
shared libraries, and every parent directory of those directories, excluding the repo root
directory. The default, auto, means true unless
<code>--incompatible_default_to_explicit_init_py</code> is used. If false, the user is
<code>--incompatible_remove_legacy_create_init</code> is used. If false, the user is
responsible for creating (possibly empty) __init__.py files and adding them to the
<code>srcs</code> of Python targets as required.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
* If requested, creates empty __init__.py files for each manifest file.
*
* <p>We do this if the rule defines {@code legacy_create_init} and its value is true. Auto is
* treated as false iff {@code --incompatible_default_to_explicit_init_py} is given.
* treated as false iff {@code --incompatible_remove_legacy_Create_init_py} is given.
*
* <p>See {@link PythonUtils#getInitPyFiles} for details about how the files are created.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
import com.google.devtools.build.lib.syntax.StarlarkValue;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.common.options.TriState;

Expand All @@ -36,7 +35,7 @@
name = "py",
doc = "A configuration fragment for Python.",
category = SkylarkModuleCategory.CONFIGURATION_FRAGMENT)
public class PythonConfiguration extends BuildConfiguration.Fragment implements StarlarkValue {
public class PythonConfiguration extends BuildConfiguration.Fragment {

private final PythonVersion version;
private final PythonVersion defaultVersion;
Expand Down Expand Up @@ -90,7 +89,8 @@ public class PythonConfiguration extends BuildConfiguration.Fragment implements
* Returns the Python version to use.
*
* <p>Specified using either the {@code --python_version} flag and {@code python_version} rule
* attribute (new API), or the {@code default_python_version} rule attribute (old API).
* attribute (new API), or the {@code --force_python} flag and {@code default_python_version} rule
* attribute (old API).
*/
public PythonVersion getPythonVersion() {
return version;
Expand Down Expand Up @@ -134,6 +134,11 @@ public String getOutputDirectoryName() {
@Override
public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {
PythonOptions opts = buildOptions.get(PythonOptions.class);
if (opts.forcePython != null && opts.incompatibleRemoveOldPythonVersionApi) {
reporter.handle(
Event.error(
"`--force_python` is disabled by `--incompatible_remove_old_python_version_api`"));
}
if (opts.incompatiblePy3IsDefault && !opts.incompatibleAllowPythonVersionTransitions) {
reporter.handle(
Event.error(
Expand Down Expand Up @@ -162,7 +167,10 @@ public boolean buildTransitiveRunfilesTrees() {
return buildTransitiveRunfilesTrees;
}

/** Returns whether use of the {@code default_python_version} attribute is allowed. */
/**
* Returns whether use of {@code --force_python} flag and {@code default_python_version} attribute
* is allowed.
*/
public boolean oldPyVersionApiAllowed() {
return oldPyVersionApiAllowed;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
package com.google.devtools.build.lib.rules.python;

import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;

Expand All @@ -39,6 +39,8 @@ public PythonConfiguration create(BuildOptions buildOptions)
pythonOptions.getDefaultPythonVersion(),
pythonOptions.buildPythonZip,
pythonOptions.buildTransitiveRunfilesTrees,
/*oldPyVersionApiAllowed=*/ !pythonOptions.incompatibleRemoveOldPythonVersionApi,
/*useNewPyVersionSemantics=*/ pythonOptions.incompatibleAllowPythonVersionTransitions,
/*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed,
/*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider,
/*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public String getTypeDescription() {
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If true, disables use of the `default_python_version` "
"If true, disables use of the `--force_python` flag and the `default_python_version` "
+ "attribute for `py_binary` and `py_test`. Use the `--python_version` flag and "
+ "`python_version` attribute instead, which have exactly the same meaning. This "
+ "flag also disables `select()`-ing over `--host_force_python`.")
Expand Down Expand Up @@ -169,17 +169,25 @@ public String getTypeDescription() {
OptionsParser.getOptionDefinitionByName(PythonOptions.class, "python_version");

/**
* Deprecated machinery for setting the Python version; will be removed soon.
* This field should be either null (unset), {@code PY2}, or {@code PY3}. Other {@code
* PythonVersion} values do not represent distinct Python versions and are not allowed.
*
* <p>This flag is not accessible to the user when {@link #incompatibleRemoveOldPythonVersionApi}
* is true.
*
* <p>Not in GraveyardOptions because we still want to prohibit users from select()ing on it.
* <p>Native rule logic should call {@link #getPythonVersion} / {@link #setPythonVersion} instead
* of accessing this option directly. BUILD/.bzl code should {@code select()} on {@code <tools
* repo>//tools/python:python_version} rather than on this option directly.
*/
@Option(
name = "force_python",
defaultValue = "null",
converter = TargetPythonVersionConverter.class,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS},
help = "No-op, will be removed soon.")
help =
"Deprecated alias for `--python_version`. Disabled by "
+ "`--incompatible_remove_old_python_version_api`.")
public PythonVersion forcePython;

private static final OptionDefinition FORCE_PYTHON_DEFINITION =
Expand Down Expand Up @@ -314,12 +322,14 @@ public PythonVersion getDefaultPythonVersion() {
/**
* Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for.
*
* <p>The version is taken as the value of {@code --python_version} if not null, otherwise {@link
* #getDefaultPythonVersion}.
* <p>The version is taken as the value of {@code --python_version} if not null, otherwise {@code
* --force_python} if not null, otherwise {@link #getDefaultPythonVersion}.
*/
public PythonVersion getPythonVersion() {
if (pythonVersion != null) {
return pythonVersion;
} else if (forcePython != null) {
return forcePython;
} else {
return getDefaultPythonVersion();
}
Expand All @@ -333,10 +343,10 @@ public PythonVersion getPythonVersion() {
* different from the existing one.
*
* <p>Under the old semantics ({@link #incompatibleAllowPythonVersionTransitions} is false),
* version transitions are not allowed once the version has already been set ({@link
* #pythonVersion} is non-null). Due to a historical bug, it is also not allowed to transition the
* version to the hard-coded default value. Under these constraints, there is only one transition
* possible, from null to the non-default value, and it is never a no-op.
* version transitions are not allowed once the version has already been set ({@link #forcePython}
* or {@link #pythonVersion} is non-null). Due to a historical bug, it is also not allowed to
* transition the version to the hard-coded default value. Under these constraints, there is only
* one transition possible, from null to the non-default value, and it is never a no-op.
*
* @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
*/
Expand All @@ -345,7 +355,7 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
if (incompatibleAllowPythonVersionTransitions) {
return !version.equals(getPythonVersion());
} else {
boolean currentlyUnset = pythonVersion == null;
boolean currentlyUnset = forcePython == null && pythonVersion == null;
boolean transitioningToNonDefault = !version.equals(getDefaultPythonVersion());
return currentlyUnset && transitioningToNonDefault;
}
Expand All @@ -362,11 +372,22 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
* <p>If the old semantics are in effect ({@link #incompatibleAllowPythonVersionTransitions} is
* false), after this method is called {@link #canTransitionPythonVersion} will return false.
*
* <p>To help avoid breaking old-API {@code select()} expressions that check the value of {@code
* "force_python"}, both the old and new flags are updated even though {@code --python_version}
* takes precedence over {@code --force_python}.
*
* @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
*/
public void setPythonVersion(PythonVersion version) {
Preconditions.checkArgument(version.isTargetValue());
this.pythonVersion = version;
// If the old version API is enabled, update forcePython for consistency. If the old API is
// disabled, don't update it because 1) no one can read it anyway, and 2) updating it during
// normalization would cause analysis-time validation of the flag to spuriously fail (it'd think
// the user set the flag).
if (!incompatibleRemoveOldPythonVersionApi) {
this.forcePython = version;
}
}

@Override
Expand Down Expand Up @@ -395,7 +416,7 @@ public FragmentOptions getHost() {
public FragmentOptions getNormalized() {
// Under the new version semantics, we want to ensure that options with "null" physical default
// values are normalized, to avoid #7808. We don't want to normalize with the old version
// semantics because that breaks backwards compatibility.
// semantics because that breaks backwards compatibility (--force_python would always be on).
PythonOptions newOptions = (PythonOptions) clone();
if (incompatibleAllowPythonVersionTransitions) {
newOptions.setPythonVersion(newOptions.getPythonVersion());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,7 @@ public void explicitInitPy_CanBeGloballyEnabled() throws Exception {
" srcs = ['foo.py'],",
")"));
useConfiguration("--incompatible_default_to_explicit_init_py=true");
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
.isEmpty();
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames()).isEmpty();
}

@Test
Expand All @@ -402,7 +401,7 @@ public void explicitInitPy_CanBeSelectivelyDisabled() throws Exception {
" legacy_create_init = True,",
")"));
useConfiguration("--incompatible_default_to_explicit_init_py=true");
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames())
.containsExactly("pkg/__init__.py");
}

Expand All @@ -416,7 +415,7 @@ public void explicitInitPy_CanBeGloballyDisabled() throws Exception {
" srcs = ['foo.py'],",
")"));
useConfiguration("--incompatible_default_to_explicit_init_py=false");
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames())
.containsExactly("pkg/__init__.py");
}

Expand All @@ -431,7 +430,6 @@ public void explicitInitPy_CanBeSelectivelyEnabled() throws Exception {
" legacy_create_init = False,",
")"));
useConfiguration("--incompatible_default_to_explicit_init_py=false");
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
.isEmpty();
assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames()).isEmpty();
}
}

0 comments on commit d1cedc0

Please sign in to comment.