Added support to expose imports for python to Skylark. by srikalyan · Pull Request #2791 · bazelbuild/bazel

srikalyan · 2017-04-07T20:20:50Z

Fixes #2617. Please let me know if you need anything else.

bazel-io · 2017-04-07T20:20:52Z

Can one of the admins verify this patch?

googlebot · 2017-04-07T20:20:54Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

srikalyan · 2017-04-07T20:22:59Z

I signed it!

googlebot · 2017-04-07T20:23:00Z

CLAs look good, thanks!

lberki · 2017-04-24T16:07:44Z

    // adjusted to be relative to the workspace name.
-    packageFragment = PathFragment.create(ruleContext.getWorkspaceName())
-        .getRelative(packageFragment);
+    return PathFragment.create(ruleContext.getWorkspaceName())


Isn't this the same as PackageIdentifier#getRunfilesPath()?

Removed it.

lberki · 2017-04-24T16:13:17Z


+  /**
+   * @return - {@link PathFragment} representing the python package.
+   */


Likewise, there is no need to put this here; since it's just PackageIdentifier#getRunfilesPath(), let's just use that and not pollute the PythonSemantics interface (which shouldn't exist anyway...)

Removed it.

lberki · 2017-04-24T16:14:44Z

            IS_USING_SHARED_LIBRARY,
-            isUsingSharedLibrary),
+            isUsingSharedLibrary,
+            PACKAGE_FRAGMENT,


Given that this is computable from the package fragment, how about not having this function here? If needed, we could add a method to Label that tells its runfiles path, right, @laurentlb ?

Removed it.

iirina · 2017-05-23T14:49:55Z

@srikalyan can you follow up on this?

srikalyan · 2017-05-23T17:39:24Z

@iirina I followed up couple days back. Please let me know if I am missing something.

iirina · 2017-05-23T18:15:20Z

I didn't see any replies to @lberki's comments and I overlooked the last commit, sorry. Can you mark the comments as done?
@lberki, can you take another look at this?

srikalyan

Addressed all the review comments.

iirina · 2017-05-23T18:32:25Z

Thank you!

lberki · 2017-05-24T06:43:41Z

+            isUsingSharedLibrary,
+            IMPORTS,
+            SkylarkNestedSet.of(String.class,
+                                makeImportStrings(imports ))


super nit: unnecessary space after "imports"

lberki · 2017-05-24T06:51:40Z

Reviewed.

bazel-io · 2017-06-12T20:04:32Z

Can one of the admins verify this patch?

lberki · 2017-05-24T06:47:31Z

+    final NestedSetBuilder<String> builder = NestedSetBuilder.compileOrder();
+
+    for (PathFragment pathFragment : imports) {
+      builder.add(pathFragment.toString());


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.

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

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

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

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?

Friendly ping @dslomov :)

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:

Use NestedSet<String> instead of NestedSet<PathFragment> everywhere.

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.

lberki · 2017-05-24T06:49:22Z

-      PythonSemantics semantics, NestedSet<Artifact> filesToBuild) {
-
+  public void addCommonTransitiveInfoProviders(
+      RuleContext ruleContext,


Why do you need RuleContext here?

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.

I can double check this and let you know.

Removed it.

dslomov · 2017-08-02T11:24:52Z

+    final NestedSetBuilder<String> builder = NestedSetBuilder.compileOrder();
+
+    for (PathFragment pathFragment : imports) {
+      builder.add(pathFragment.toString());


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

hlopko · 2017-10-11T12:22:21Z

Friendly ping @srikalyan , is this still being developed?

bazel-io · 2017-10-11T12:22:23Z

Can one of the admins verify this patch?

ali5h · 2017-12-08T22:09:27Z

any update on this?

…e existing code.

fahhem · 2017-12-12T21:38:46Z

Any chance this is getting in in 2017?

srikalyan · 2017-12-18T20:10:13Z

@fahhem I am waiting on the review comments

fahhem · 2017-12-19T10:23:18Z

@srikalyan, yes, I was hoping to prod the bazel maintainers more than you. Sorry for that! @dslomov or @vladmos, ping! Is there a plan to make something more general available soon or can we merge this PR for now? Unfortunately, we missed the 0.9.0 release, but hopefully we can get in before the next one still!

jin · 2018-01-08T17:32:54Z

@dslomov PTAL at the review comments when you have the bandwidth?

philwo · 2018-02-03T10:28:25Z

Ping @dslomov - please take action on this PR.

bazel-io · 2018-02-03T10:28:27Z

Can one of the admins verify this patch?

laszlocsomor · 2018-03-09T13:37:57Z

ping -- are there any updates? can we merge or close this PR?

srikalyan · 2018-03-16T23:33:40Z

I think it is safe to merge. I am just waiting for approval :(

dslomov · 2018-03-19T16:25:12Z

+    final NestedSetBuilder<String> builder = NestedSetBuilder.compileOrder();
+
+    for (PathFragment pathFragment : imports) {
+      builder.add(pathFragment.toString());


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:

Use NestedSet<String> instead of NestedSet<PathFragment> everywhere.

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.

lfpino · 2018-03-27T13:45:43Z

Hi @srikalyan, Bazel sheriff here, what's the status of this pull request? It hasn't been updated for more than a week so it'd be good to understand if it's still ongoing or safe to close. Thanks.

jin · 2018-04-09T06:16:54Z

@srikalyan ping, could you please address @dslomov's comments? thanks!

c4urself · 2018-04-18T14:21:30Z

A lot of people are wasting cycles on this, trying to reimplement all the python rules in Skylark to be able to access their version of the imports attribute, see for example: https://github.com/TriggerMail/rules_pyz/blob/master/rules_python_zip/rules_python_zip.bzl#L39 anything we can do to move this forward?

ulfjack · 2018-10-11T02:30:31Z

Closing due to lack of updates from the original author. Maybe someone else wants to take this over.

Added support to expose imports for python to Skylark.

8d8eae9

Fixes bazelbuild#2617.

googlebot added the cla: no label Apr 7, 2017

googlebot added cla: yes and removed cla: no labels Apr 7, 2017

srikalyan added 2 commits April 7, 2017 13:32

Added finals.

e0d301b

Merge remote-tracking branch 'upstream/master'

98c25c3

buchgr requested a review from vladmos April 10, 2017 11:55

buchgr assigned vladmos Apr 10, 2017

srikalyan mentioned this pull request Apr 10, 2017

Added support to honor python imports rules for pex_binaries. benley/bazel_rules_pex#35

Open

vladmos assigned lberki Apr 24, 2017

lberki reviewed Apr 24, 2017

View reviewed changes

Removed the pyfragments info as that can retrieved easily.

01f59a7

srikalyan commented May 23, 2017

View reviewed changes

lberki reviewed May 24, 2017

View reviewed changes

Fixed the format on the code.

ff966ff

benley mentioned this pull request Jun 17, 2017

py_library imports ignored when linking into pex_binary benley/bazel_rules_pex#36

Open

lberki reviewed Jun 26, 2017

View reviewed changes

hlopko added the category: extensibility > skylark label Jun 26, 2017

Merge remote-tracking branch 'upstream/master'

a94b859

dslomov suggested changes Aug 2, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master'

a8da0d6

Addressed the code review comments and formatted the code based on th…

1507e03

…e existing code.

dslomov suggested changes Mar 19, 2018

View reviewed changes

irengrig mentioned this pull request Sep 4, 2018

continue making py_ rules import attribute visible in Starlark srikalyan/bazel#1

Open

ulfjack closed this Oct 11, 2018

aiuto added team-Starlark and removed z-category: extensibility > skylark labels Feb 4, 2019

Uh oh!

Conversation

srikalyan commented Apr 7, 2017

Uh oh!

bazel-io commented Apr 7, 2017

Uh oh!

googlebot commented Apr 7, 2017

Uh oh!

srikalyan commented Apr 7, 2017

Uh oh!

googlebot commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iirina commented May 23, 2017

Uh oh!

srikalyan commented May 23, 2017

Uh oh!

iirina commented May 23, 2017

Uh oh!

srikalyan left a comment

Choose a reason for hiding this comment

Uh oh!

iirina commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lberki commented May 24, 2017

Uh oh!

bazel-io commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hlopko commented Oct 11, 2017

Uh oh!

bazel-io commented Oct 11, 2017

Uh oh!

ali5h commented Dec 8, 2017

Uh oh!

fahhem commented Dec 12, 2017

Uh oh!

srikalyan commented Dec 18, 2017

Uh oh!

fahhem commented Dec 19, 2017

Uh oh!

jin commented Jan 8, 2018