Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support to expose imports for python to Skylark. #2791

Closed
wants to merge 8 commits into from
Closed

Added support to expose imports for python to Skylark. #2791

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d8eae9
Added support to expose imports for python to Skylark.
srikalyan Apr 7, 2017
e0d301b
Added finals.
srikalyan Apr 7, 2017
98c25c3
Merge remote-tracking branch 'upstream/master'
srikalyan Apr 7, 2017
01f59a7
Removed the pyfragments info as that can retrieved easily.
srikalyan May 17, 2017
ff966ff
Fixed the format on the code.
srikalyan Jun 12, 2017
a94b859
Merge remote-tracking branch 'upstream/master'
srikalyan Jul 10, 2017
a8da0d6
Merge remote-tracking branch 'upstream/master'
srikalyan Dec 7, 2017
1507e03
Addressed the code review comments and formatted the code based on th…
srikalyan Dec 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 36 additions & 6 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ 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";

private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() {
@Override
Expand Down Expand Up @@ -132,9 +133,12 @@ public void initBinary(List<Artifact> srcs) {
addPyExtraActionPseudoAction();
}

public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder,
PythonSemantics semantics, NestedSet<Artifact> filesToBuild) {

public void addCommonTransitiveInfoProviders(
RuleContext ruleContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need RuleContext here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't exactly remember as this has been really old. I think I had to add it because rule context in pycommon was different from pylibrary/pybinary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can double check this and let you know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

RuleConfiguredTargetBuilder builder,
PythonSemantics semantics,
NestedSet<Artifact> filesToBuild,
NestedSet<PathFragment> imports) {
builder
.add(
InstrumentedFilesProvider.class,
Expand All @@ -145,7 +149,10 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder
filesToBuild))
.addSkylarkTransitiveInfo(
PYTHON_SKYLARK_PROVIDER_NAME,
createSourceProvider(this.transitivePythonSources, usesSharedLibraries()))
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)
Expand All @@ -158,16 +165,39 @@ public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder
* addSkylarkTransitiveInfo(PYTHON_SKYLARK_PROVIDER_NAME, createSourceProvider(...))
*/
public static SkylarkClassObject createSourceProvider(
NestedSet<Artifact> transitivePythonSources, boolean isUsingSharedLibrary) {
NestedSet<Artifact> transitivePythonSources,
boolean isUsingSharedLibrary,
NestedSet<PathFragment> imports) {
return NativeClassObjectConstructor.STRUCT.create(
ImmutableMap.<String, Object>of(
TRANSITIVE_PYTHON_SRCS,
SkylarkNestedSet.of(Artifact.class, transitivePythonSources),
IS_USING_SHARED_LIBRARY,
isUsingSharedLibrary),
isUsingSharedLibrary,
IMPORTS,
SkylarkNestedSet.of(String.class,
makeImportStrings(imports ))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: unnecessary space after "imports"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

),
"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<String> makeImportStrings(
NestedSet<PathFragment> imports) {
final NestedSetBuilder<String> builder = NestedSetBuilder.compileOrder();

for (PathFragment pathFragment : imports) {
builder.add(pathFragment.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This effectively negates any benefit of nested sets here, because you create a new, flat nested set with the strings instead of the carefully built-up nested set with import path fragments inherited from dependent targets.

@laurentlb @vladmos , is there a way to expose a NestedSet<PathFragment> to Skylark instead? That's a more promising avenue than working around the limitation that PathFragments aren't available there. This would also help with flattening the PathFragments into strings so that e.g. "longdirectory/a" and "longdirectory/b" can share structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for late reply. We do not plan to expose PathFragments to Skylark. Can you do with NestedSet<String> instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or return a Iterable that lazily iterates a nested set when needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late reply. I will add it asap.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for not understanding what you are trying to suggest here. I think (I may be completely wrong) that you are suggesting the proposed change? @dslomov can you comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping @dslomov :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read https://docs.bazel.build/versions/master/skylark/depsets.html.

What you are doing here is flattening the depset of path fragments to convert them to strings.
Since this is happening for every python rule, that will bloat the memory.
There are two approaches you can take instead:

  1. Use NestedSet<String> instead of NestedSet<PathFragment> everywhere.
  2. Make a value of 'imports' an Iterable that lazily flattens a nested set on iteration.

My preferred approach is (1) but I do not know how expensive it is to keep Strings instead of PathFragments.
(2) is ok as well.

}

return builder.build();
}

public PythonVersion getDefaultPythonVersion() {
return ruleContext.getRule()
.isAttrDefined("default_python_version", Type.STRING)
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down