From 8d8eae9080ce653370fed1b9c7fb7b89185da415 Mon Sep 17 00:00:00 2001 From: Srikalyan Swayampakula Date: Fri, 7 Apr 2017 13:19:00 -0700 Subject: [PATCH 1/5] Added support to expose imports for python to Skylark. Fixes #2617. --- .../rules/python/BazelPythonSemantics.java | 15 ++++-- .../build/lib/rules/python/PyBinary.java | 3 +- .../build/lib/rules/python/PyCommon.java | 47 ++++++++++++++++--- .../build/lib/rules/python/PyLibrary.java | 3 +- .../lib/rules/python/PythonSemantics.java | 5 ++ 5 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 89778536da68a6..7a19fd5cbaab4f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -81,13 +81,18 @@ public Collection precompiledPythonFiles( } @Override - public List getImports(RuleContext ruleContext) { - List result = new ArrayList<>(); - PathFragment packageFragment = ruleContext.getLabel().getPackageIdentifier().getRunfilesPath(); + public PathFragment getPackageFragment(RuleContext ruleContext) { + final PathFragment packageFragment = ruleContext.getLabel().getPackageIdentifier().getRunfilesPath(); // Python scripts start with x.runfiles/ as the module space, so everything must be manually // adjusted to be relative to the workspace name. - packageFragment = new PathFragment(ruleContext.getWorkspaceName()) - .getRelative(packageFragment); + return new PathFragment(ruleContext.getWorkspaceName()) + .getRelative(packageFragment); + } + + @Override + public List getImports(RuleContext ruleContext) { + List result = new ArrayList<>(); + PathFragment packageFragment = getPackageFragment(ruleContext); for (String importsAttr : ruleContext.attributes().get("imports", Type.STRING_LIST)) { importsAttr = ruleContext.expandMakeVariables("includes", importsAttr); if (importsAttr.startsWith("/")) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java index a04a0eda114e2c..605779ad433c00 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java @@ -104,7 +104,8 @@ static RuleConfiguredTargetBuilder init(RuleContext ruleContext, PythonSemantics RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - common.addCommonTransitiveInfoProviders(builder, semantics, common.getFilesToBuild()); + common.addCommonTransitiveInfoProviders( + ruleContext, builder, semantics, common.getFilesToBuild(), imports); semantics.postInitBinary(ruleContext, runfilesSupport, common); return builder 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 a34388381648c3..e763a4f704ee02 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 @@ -69,6 +69,8 @@ public final class PyCommon { public static final String PYTHON_SKYLARK_PROVIDER_NAME = "py"; public static final String TRANSITIVE_PYTHON_SRCS = "transitive_sources"; public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries"; + public static final String IMPORTS = "imports"; + public static final String PACKAGE_FRAGMENT = "package_fragment"; private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() { @Override @@ -132,9 +134,12 @@ public void initBinary(List srcs) { addPyExtraActionPseudoAction(); } - public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder, - PythonSemantics semantics, NestedSet filesToBuild) { - + public void addCommonTransitiveInfoProviders( + RuleContext ruleContext, + RuleConfiguredTargetBuilder builder, + PythonSemantics semantics, + NestedSet filesToBuild, + NestedSet imports) { builder .add( InstrumentedFilesProvider.class, @@ -145,7 +150,11 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder filesToBuild)) .addSkylarkTransitiveInfo( PYTHON_SKYLARK_PROVIDER_NAME, - createSourceProvider(this.transitivePythonSources, usesSharedLibraries())) + createSourceProvider( + transitivePythonSources, + usesSharedLibraries(), + semantics.getPackageFragment(ruleContext), + imports)) // Python targets are not really compilable. The best we can do is make sure that all // generated source files are ready. .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, transitivePythonSources) @@ -158,16 +167,42 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder * addSkylarkTransitiveInfo(PYTHON_SKYLARK_PROVIDER_NAME, createSourceProvider(...)) */ public static SkylarkClassObject createSourceProvider( - NestedSet transitivePythonSources, boolean isUsingSharedLibrary) { + NestedSet transitivePythonSources, + boolean isUsingSharedLibrary, + PathFragment packageFragment, + NestedSet imports) { return NativeClassObjectConstructor.STRUCT.create( ImmutableMap.of( TRANSITIVE_PYTHON_SRCS, SkylarkNestedSet.of(Artifact.class, transitivePythonSources), IS_USING_SHARED_LIBRARY, - isUsingSharedLibrary), + isUsingSharedLibrary, + PACKAGE_FRAGMENT, + packageFragment.toString(), + IMPORTS, + SkylarkNestedSet.of(String.class, + makeImportStrings(imports )) + ), "No such attribute '%s'"); } + /** + * Converts {@link NestedSet} of {@link PathFragment} to + * {@link NestedSet} of {@link String} + * @param imports - {@link NestedSet} of {@link PathFragment} + * @return - {@link NestedSet} of {@link String} + */ + public static NestedSet makeImportStrings( + NestedSet imports) { + final NestedSetBuilder builder = NestedSetBuilder.compileOrder(); + + for (PathFragment pathFragment : imports) { + builder.add(pathFragment.toString()); + } + + return builder.build(); + } + public PythonVersion getDefaultPythonVersion() { return ruleContext.getRule() .isAttrDefined("default_python_version", Type.STRING) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java index 8d20b9657b0aca..446e1648d3c02d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java @@ -88,7 +88,8 @@ protected void collect(CcLinkParams.Builder builder, boolean linkingStatically, runfilesBuilder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES); RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - common.addCommonTransitiveInfoProviders(builder, semantics, filesToBuild); + common.addCommonTransitiveInfoProviders( + ruleContext, builder, semantics, filesToBuild, imports); return builder .setFilesToBuild(filesToBuild) .add(RunfilesProvider.class, RunfilesProvider.simple(runfilesBuilder.build())) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java index 08fcb1b4658635..6fa89d49dd35ff 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java @@ -56,6 +56,11 @@ void collectDefaultRunfilesForBinary(RuleContext ruleContext, Runfiles.Builder b Collection precompiledPythonFiles( RuleContext ruleContext, Collection sources, PyCommon common); + /** + * @return - {@link PathFragment} representing the python package. + */ + PathFragment getPackageFragment(RuleContext ruleContext); + /** * Returns a list of PathFragments for the import paths specified in the imports attribute. */ From e0d301bb1da8f7a0a77c429cb34cc16da62d44ee Mon Sep 17 00:00:00 2001 From: Srikalyan Swayampakula Date: Fri, 7 Apr 2017 13:32:05 -0700 Subject: [PATCH 2/5] Added finals. --- .../build/lib/bazel/rules/python/BazelPythonSemantics.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 7a19fd5cbaab4f..95eb5c1038ca98 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -91,8 +91,8 @@ public PathFragment getPackageFragment(RuleContext ruleContext) { @Override public List getImports(RuleContext ruleContext) { - List result = new ArrayList<>(); - PathFragment packageFragment = getPackageFragment(ruleContext); + final List result = new ArrayList<>(); + final PathFragment packageFragment = getPackageFragment(ruleContext); for (String importsAttr : ruleContext.attributes().get("imports", Type.STRING_LIST)) { importsAttr = ruleContext.expandMakeVariables("includes", importsAttr); if (importsAttr.startsWith("/")) { From 01f59a789de924b054fbc207a69429272828997e Mon Sep 17 00:00:00 2001 From: Srikalyan Swayampakula Date: Wed, 17 May 2017 10:34:58 -0700 Subject: [PATCH 3/5] Removed the pyfragments info as that can retrieved easily. --- .../bazel/rules/python/BazelPythonSemantics.java | 16 +++++----------- .../build/lib/rules/python/PyCommon.java | 5 ----- .../build/lib/rules/python/PythonSemantics.java | 5 ----- 3 files changed, 5 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 4ad2c70a95fc42..ac6d574c93580c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -87,19 +87,13 @@ public Collection precompiledPythonFiles( } @Override - public PathFragment getPackageFragment(RuleContext ruleContext) { - final PathFragment packageFragment = ruleContext.getLabel().getPackageIdentifier().getRunfilesPath(); + public List getImports(RuleContext ruleContext) { + List result = new ArrayList<>(); + PathFragment packageFragment = ruleContext.getLabel().getPackageIdentifier().getRunfilesPath(); // Python scripts start with x.runfiles/ as the module space, so everything must be manually // adjusted to be relative to the workspace name. - return PathFragment.create(ruleContext.getWorkspaceName()) - .getRelative(packageFragment); - } - - @Override - public List getImports(RuleContext ruleContext) { - final List result = new ArrayList<>(); - final PathFragment packageFragment = getPackageFragment(ruleContext); - + packageFragment = PathFragment.create(ruleContext.getWorkspaceName()) + .getRelative(packageFragment); for (String importsAttr : ruleContext.attributes().get("imports", Type.STRING_LIST)) { importsAttr = ruleContext.expandMakeVariables("includes", importsAttr); if (importsAttr.startsWith("/")) { 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 0cbd8e4e15f874..03ada6c669ddae 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 @@ -70,7 +70,6 @@ public final class PyCommon { public static final String TRANSITIVE_PYTHON_SRCS = "transitive_sources"; public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries"; public static final String IMPORTS = "imports"; - public static final String PACKAGE_FRAGMENT = "package_fragment"; private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() { @Override @@ -153,7 +152,6 @@ public void addCommonTransitiveInfoProviders( createSourceProvider( transitivePythonSources, usesSharedLibraries(), - semantics.getPackageFragment(ruleContext), imports)) // Python targets are not really compilable. The best we can do is make sure that all // generated source files are ready. @@ -169,7 +167,6 @@ public void addCommonTransitiveInfoProviders( public static SkylarkClassObject createSourceProvider( NestedSet transitivePythonSources, boolean isUsingSharedLibrary, - PathFragment packageFragment, NestedSet imports) { return NativeClassObjectConstructor.STRUCT.create( ImmutableMap.of( @@ -177,8 +174,6 @@ public static SkylarkClassObject createSourceProvider( SkylarkNestedSet.of(Artifact.class, transitivePythonSources), IS_USING_SHARED_LIBRARY, isUsingSharedLibrary, - PACKAGE_FRAGMENT, - packageFragment.toString(), IMPORTS, SkylarkNestedSet.of(String.class, makeImportStrings(imports )) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java index 41569d9f15d31d..bbaaa8f423c283 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSemantics.java @@ -59,11 +59,6 @@ void collectDefaultRunfilesForBinary(RuleContext ruleContext, Runfiles.Builder b Collection precompiledPythonFiles( RuleContext ruleContext, Collection sources, PyCommon common); - /** - * @return - {@link PathFragment} representing the python package. - */ - PathFragment getPackageFragment(RuleContext ruleContext); - /** * Returns a list of PathFragments for the import paths specified in the imports attribute. */ From ff966ffb8384d0366082d3f676e5d2f864135bc7 Mon Sep 17 00:00:00 2001 From: Srikalyan Swayampakula Date: Mon, 12 Jun 2017 13:03:58 -0700 Subject: [PATCH 4/5] Fixed the format on the code. --- .../com/google/devtools/build/lib/rules/python/PyCommon.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 03ada6c669ddae..4c32fe05bc0b0f 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 @@ -175,8 +175,7 @@ public static SkylarkClassObject createSourceProvider( IS_USING_SHARED_LIBRARY, isUsingSharedLibrary, IMPORTS, - SkylarkNestedSet.of(String.class, - makeImportStrings(imports )) + SkylarkNestedSet.of(String.class, makeImportStrings(imports)) ), "No such attribute '%s'"); } From 1507e034262e4de7c427d2b3594186071b157a53 Mon Sep 17 00:00:00 2001 From: Srikalyan Swayampakula Date: Fri, 8 Dec 2017 17:29:27 -0800 Subject: [PATCH 5/5] Addressed the code review comments and formatted the code based on the existing code. --- .../devtools/build/lib/rules/python/PyBinary.java | 3 +-- .../devtools/build/lib/rules/python/PyCommon.java | 11 ++++------- .../devtools/build/lib/rules/python/PyLibrary.java | 3 +-- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java index f0ff633e4afe14..182545f4c066e9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java @@ -117,8 +117,7 @@ static RuleConfiguredTargetBuilder init(RuleContext ruleContext, PythonSemantics RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - common.addCommonTransitiveInfoProviders( - ruleContext, builder, semantics, common.getFilesToBuild(), imports); + common.addCommonTransitiveInfoProviders(builder, semantics, common.getFilesToBuild(), imports); semantics.postInitBinary(ruleContext, runfilesSupport, common); return builder 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 489a7ba4d04c4b..1f14d026d1464d 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 @@ -155,8 +155,7 @@ public Artifact getPythonZipArtifact(Artifact executable) { } public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder, - PythonSemantics semantics, NestedSet filesToBuild, - NestedSet imports) { + PythonSemantics semantics, NestedSet filesToBuild, NestedSet imports) { builder .add( @@ -168,10 +167,7 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder filesToBuild)) .addSkylarkTransitiveInfo( PYTHON_SKYLARK_PROVIDER_NAME, - createSourceProvider( - transitivePythonSources, - usesSharedLibraries(), - imports)) + createSourceProvider(transitivePythonSources, usesSharedLibraries(), imports)) // Python targets are not really compilable. The best we can do is make sure that all // generated source files are ready. .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, transitivePythonSources) @@ -184,7 +180,8 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder *

addSkylarkTransitiveInfo(PYTHON_SKYLARK_PROVIDER_NAME, createSourceProvider(...)) */ public static Info createSourceProvider( - NestedSet transitivePythonSources, boolean isUsingSharedLibrary, NestedSet imports) { + NestedSet transitivePythonSources, boolean isUsingSharedLibrary, + NestedSet imports) { return NativeProvider.STRUCT.create( ImmutableMap.of( TRANSITIVE_PYTHON_SRCS, diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java index 3bde081e2c0668..ef8721d790e8c7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java @@ -88,8 +88,7 @@ protected void collect(CcLinkParams.Builder builder, boolean linkingStatically, runfilesBuilder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES); RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - common.addCommonTransitiveInfoProviders( - ruleContext, builder, semantics, filesToBuild, imports); + common.addCommonTransitiveInfoProviders(builder, semantics, filesToBuild, imports); return builder .setFilesToBuild(filesToBuild) .add(RunfilesProvider.class, RunfilesProvider.simple(runfilesBuilder.build()))