From a352207beee21d17b4871c96c687e3b6cacca1fc Mon Sep 17 00:00:00 2001 From: brandjon Date: Fri, 1 Feb 2019 07:14:29 -0800 Subject: [PATCH] Update documentation for Python version mechanisms This isn't perfect in terms of linkifying names of rules/attrs, but it's a vast improvement and needed to get this change migration-ready. Ideally we'd have a dedicated Python concepts page. Also did a little drive-by cleanup of unrelated attributes' docs and a few redundancies in attribute declarations. Work toward #6583, #6442. RELNOTES: None PiperOrigin-RevId: 231966448 --- .../rules/python/BazelPyLibraryRule.java | 25 +--- .../rules/python/BazelPyRuleClasses.java | 114 ++++++++++-------- .../build/lib/rules/python/PyCommon.java | 12 +- .../build/lib/rules/python/PythonOptions.java | 49 ++++---- 4 files changed, 101 insertions(+), 99 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyLibraryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyLibraryRule.java index c2a80a46fede63..caea8a9e5e17f8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyLibraryRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyLibraryRule.java @@ -31,30 +31,15 @@ public final class BazelPyLibraryRule implements RuleDefinition { public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) { return builder .requiresConfigurationFragments(PythonConfiguration.class) - /* - The list of other libraries to be linked in to the library target. - See general comments about deps at - - Attributes common to all build rules. - In practice, these arguments are treated like those in srcs; - you may move items between these lists willy-nilly. It's probably more - readable to keep your .py files in your srcs. - */ - - /* - The list of files needed by this library at runtime. - See general comments about data at - - Attributes common to all build rules. - */ /* - The list of source files that are processed to create the target. + The list of source (.py) files that are processed to create the target. This includes all your checked-in code and any generated source files. */ - .add(attr("srcs", LABEL_LIST) - .direct_compile_time_input() - .allowedFileTypes(BazelPyRuleClasses.PYTHON_SOURCE)) + .add( + attr("srcs", LABEL_LIST) + .direct_compile_time_input() + .allowedFileTypes(BazelPyRuleClasses.PYTHON_SOURCE)) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java index 0d4a0980a9f3da..e391f530bcf351 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java @@ -65,8 +65,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) See general comments about deps at Attributes common to all build rules. - These can be - py_binary rules, + These are generally py_library rules. */ .override( @@ -80,7 +79,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) SkylarkProviderIdentifier.forLegacy(PyStructUtils.PROVIDER_NAME)), // Modern provider. ImmutableList.of(PyInfo.PROVIDER.id()))) - .legacyMandatoryProviders(PyStructUtils.PROVIDER_NAME) .allowedFileTypes()) /* List of import directories to be added to the PYTHONPATH. @@ -97,26 +95,35 @@ Absolute paths (paths that start with /) and paths that references */ .add(attr("imports", STRING_LIST).value(ImmutableList.of())) /* - The value set here is for documentation purpose, and it will NOT determine which version - of python interpreter to use. Starting with 0.5.3 this attribute has been deprecated and - no longer has effect. - A string specifying the Python major version(s) that the .py source - files listed in the srcs of this rule are compatible with. - Please reference to py_runtime rules for - determining the python version. - Valid values are:
- "PY2ONLY" - - Python 2 code that is not suitable for 2to3 conversion.
- "PY2" - - Python 2 code that is expected to work when run through 2to3.
- "PY2AND3" - - Code that is compatible with both Python 2 and 3 without - 2to3 conversion.
- "PY3ONLY" - - Python 3 code that will not run on Python 2.
- "PY3" - - A synonym for PY3ONLY.
-
+ This attribute declares the target's srcs to be compatible with either Python + 2, Python 3, or both. To actually set the Python runtime version, use the + python_version attribute of an + executable Python rule (py_binary or py_test). + +

Allowed values are: "PY2AND3", "PY2", and "PY3". + The values "PY2ONLY" and "PY3ONLY" are also allowed for historic + reasons, but they are essentially the same as "PY2" and "PY3" + and should be avoided. + +

Under the old semantics + (--experimental_allow_python_version_transitions=false), it is an error to + build any Python target for a version disallowed by its srcs_version + attribute. Under the new semantics + (--experimental_allow_python_version_transitions=true), this check is + deferred to the executable rule: You can build a srcs_version = "PY3" + py_library target for Python 2, but you cannot actually depend on it via + deps from a Python 3 py_binary. + +

To get diagnostic information about which dependencies introduce version requirements, + you can run the find_requirements aspect on your target: + 

+          bazel build <your target> \
+              --aspects=@bazel_tools//tools/python:srcs_version.bzl%find_requirements \
+              --output_groups=pyversioninfo
+
+ This will build a file with the suffix -pyversioninfo.txt giving information + about why your target requires one Python version or another. Note that it works even if + the given target failed to build due to a version conflict. */ .add( attr("srcs_version", STRING) @@ -154,15 +161,6 @@ public static final class PyBinaryBaseRule implements RuleDefinition { @Override public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironment env) { return builder - /* - The list of files needed by this binary at runtime. - See general comments about data at - - Attributes common to all build rules. - Also see the data argument of - the py_library rule for details. - */ - /* The name of the source file that is the main entry point of the application. This file must also be listed in srcs. If left unspecified, @@ -171,12 +169,10 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen */ .add(attr("main", LABEL).allowedFileTypes(PYTHON_SOURCE)) /* - A string specifying the default Python major version to use when building this binary and - all of its deps. - Valid values are "PY2" (default) or "PY3". - Python 3 support is experimental. -

If both this attribute and python_version are supplied, - default_python_version will be ignored. + A deprecated alias for python_version; use that instead. This attribute is + disabled under --experimental_remove_old_python_version_api. For migration + purposes, if python_version is given then the value of + default_python_version is ignored. */ .add( attr(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, STRING) @@ -186,8 +182,25 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access" + " to the configuration")) /* - A replacement for default_python_version. If both this and - default_python_version are supplied, the latter will be ignored. + Whether to build this target (and its transitive deps) for Python 2 or Python + 3. Valid values are "PY2" (the default) and "PY3". + +

Under the old semantics + (--experimental_allow_python_version_transitions=false), the Python version + generally cannot be changed once set. This means that the --python_version + flag overrides this attribute, and other Python binaries in the data deps of + this target are forced to use the same version as this target. + +

Under the new semantics + (--experimental_allow_python_version_transitions=true), the Python version + is always set (possibly by default) to whatever version is specified by this attribute, + regardless of the version specified on the command line or by other targets that depend on + this one. + +

If you want to select() on the current Python version, you can inspect the + value of @bazel_tools//tools/python:python_version. See + here + for more information. */ .add( attr(PyCommon.PYTHON_VERSION_ATTRIBUTE, STRING) @@ -197,26 +210,23 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access" + " to the configuration")) /* - The list of source files that are processed to create the target. - This includes all your checked-in code and any - generated source files. The line between srcs and - deps is loose. The .py files - probably belong in srcs and library targets probably belong - in deps, but don't worry about it too much. + The list of source (.py) files that are processed to create the target. + This includes all your checked-in code and any generated source files. Library targets + belong in deps instead, while other binary files needed at runtime belong in + data. */ .add( attr("srcs", LABEL_LIST) .mandatory() .allowedFileTypes(PYTHON_SOURCE) - .direct_compile_time_input() - .allowedFileTypes(BazelPyRuleClasses.PYTHON_SOURCE)) + .direct_compile_time_input()) /* Whether to implicitly create empty __init__.py files in the runfiles tree. These are created in every directory containing Python source code or - shared libraries, and every parent directory of those directories. - Default is true for backward compatibility. If false, the user is responsible - for creating __init__.py files (empty or not) and adding them to `srcs` or `deps` - of Python targets as required. + shared libraries, and every parent directory of those directories, excluding the repo root + directory. The default is true for backward compatibility. If false, the user is + responsible for creating (possibly empty) __init__.py files and adding them to the + srcs of Python targets as required. */ .add(attr("legacy_create_init", BOOLEAN).value(true)) /* diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index 4a46cc52306c6e..6e12259516c5e9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -483,14 +483,16 @@ private boolean maybeCreateFailActionDueToTransitiveSourcesVersion() { if (!ruleContext.getFragment(PythonConfiguration.class).useNewPyVersionSemantics()) { return false; } - // TODO(brandjon): Add link to documentation explaining the error and use of the aspect. + String errorTemplate = + "This target is being built for Python %s but (transitively) includes Python %s-only " + + "sources. You can get diagnostic information about which dependencies introduce this " + + "version requirement by running the `find_requirements` aspect. For more info see " + + "the documentation for the `srcs_version` attribute."; String error = null; if (version == PythonVersion.PY2 && hasPy3OnlySources) { - error = - "target is being built for Python 2 but (transitively) includes Python 3-only sources"; + error = String.format(errorTemplate, "2", "3"); } else if (version == PythonVersion.PY3 && hasPy2OnlySources) { - error = - "target is being built for Python 3 but (transitively) includes Python 2-only sources"; + error = String.format(errorTemplate, "3", "2"); } if (error == null) { return false; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index 1c8a9e40c39593..09814484f688ed 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -69,30 +69,28 @@ public String getTypeDescription() { help = "Build python executable zip; on on Windows, off on other platforms") public TriState buildPythonZip; - // TODO(brandjon): For both experimental options below, add documentation and add a link to it - // from the help text, then change the documentationCategory to SKYLARK_SEMANTICS. - @Option( name = "experimental_remove_old_python_version_api", - // TODO(brandjon): Do not flip until we have an answer for how to disallow the - // "default_python_version" attribute without hacking up native.existing_rules(). See - // #7071 and b/122596733. defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, metadataTags = {OptionMetadataTag.EXPERIMENTAL}, help = "If true, disables use of the `--force_python` flag and the `default_python_version` " - + "attribute for `py_binary` and `py_test`.") + + "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`.") public boolean experimentalRemoveOldPythonVersionApi; @Option( name = "experimental_allow_python_version_transitions", defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, metadataTags = {OptionMetadataTag.EXPERIMENTAL}, - help = "If true, Python rules use the new PY2/PY3 version semantics.") + help = + "If true, Python rules use the new PY2/PY3 version semantics. For more information, see " + + "the documentation for `py_binary`'s `python_version` attribute.") public boolean experimentalAllowPythonVersionTransitions; /** @@ -107,17 +105,16 @@ public String getTypeDescription() { name = "python_version", defaultValue = "null", converter = TargetPythonVersionConverter.class, - // TODO(brandjon): Change to OptionDocumentationCategory.GENERIC_INPUTS when this is - // sufficiently implemented/documented. - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + documentationCategory = OptionDocumentationCategory.GENERIC_INPUTS, effectTags = { OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS // because of "-py3" output root }, help = - "The Python major version mode, either `PY2` or `PY3`. Note that this is overridden by " - + "`py_binary` and `py_test` targets (whether or not they specify their version " - + "explicitly), so there is usually not much reason to supply this flag.") + "The Python major version mode, either `PY2` or `PY3`. Note that under the new version " + + "semantics (`--experimental_allow_python_version_transitions`) this is overridden " + + "by `py_binary` and `py_test` targets (even if they don't explicitly specify a " + + "version) so there is usually not much reason to supply this flag.") public PythonVersion pythonVersion; private static final OptionDefinition PYTHON_VERSION_DEFINITION = @@ -140,7 +137,9 @@ public String getTypeDescription() { converter = TargetPythonVersionConverter.class, documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, - help = "Overrides default_python_version attribute. Can be \"PY2\" or \"PY3\".") + help = + "Deprecated alias for `--python_version`. Disabled by " + + "`--experimental_remove_old_python_version_api`.") public PythonVersion forcePython; private static final OptionDefinition FORCE_PYTHON_DEFINITION = @@ -186,19 +185,25 @@ public String getTypeDescription() { @Override public Map getSelectRestrictions() { - // TODO(brandjon): Add an error string that references documentation explaining to use - // @bazel_tools//tools/python:python_version instead. + // TODO(brandjon): Instead of referencing the python_version target, whose path depends on the + // tools repo name, reference a standalone documentation page instead. ImmutableMap.Builder restrictions = ImmutableMap.builder(); restrictions.put( PYTHON_VERSION_DEFINITION, - new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null)); + new SelectRestriction( + /*visibleWithinToolsPackage=*/ true, + "Use @bazel_tools//python/tools:python_version instead.")); if (experimentalRemoveOldPythonVersionApi) { restrictions.put( FORCE_PYTHON_DEFINITION, - new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null)); + new SelectRestriction( + /*visibleWithinToolsPackage=*/ true, + "Use @bazel_tools//python/tools:python_version instead.")); restrictions.put( HOST_FORCE_PYTHON_DEFINITION, - new SelectRestriction(/*visibleWithinToolsPackage=*/ false, /*errorMessage=*/ null)); + new SelectRestriction( + /*visibleWithinToolsPackage=*/ false, + "Use @bazel_tools//python/tools:python_version instead.")); } return restrictions.build(); }