pip: entry_point: Add support for exit codes by gibfahn · Pull Request #550 · bazel-contrib/rules_python

gibfahn · 2021-10-13T11:15:14Z

PR Checklist

Please check if your PR fulfills the following requirements:

Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Entry point used via the entry_point helper, if a function returning a int, the return int does not become the program return code.

What is the new behavior?

Entry points are expected to return an int and this int is the status code.

Does this PR introduce a breaking change?

Yes
No

Other information

I tried looking for the code in setuptool that generates the shim but did not find it proper. The closest I found was setuptools/command/easy_install.py.

My use case is the sphinx-build entry point that maps to the following code:

def main(argv: List[str] = sys.argv[1:]) -> int:
    # ...

Using pipenv install the following shim gets generated:

#!SOMELOCATION/python
# -*- coding: utf-8 -*-
import re
import sys
from sphinx.cmd.build import main
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

This does forward the error to sys.exit. In the case of rules_python, I am not sure we need the regex bit, but I do know.

Commits (oldest to newest)

bc30a71 pip: entry_point: Add support for exit codes

4a044b5 Add test for entrypoint exit codes

cbe7c1a Update tests for entrypoint exit codes

google-cla · 2021-10-14T11:27:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

UebelAndre · 2021-10-18T21:17:33Z

This seems exciting! Any updates here?

hrfuller

Overall seems like a reasonable change. There is some fit and finish to do before merging.

hrfuller · 2021-10-18T22:18:55Z

 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

-http_archive(
+local_repository(


Don't commit this. The integration test runner replaces the url here with a file:// url to a tar of the current version.

Ahh, thanks, reverted.

hrfuller · 2021-10-18T22:20:00Z


 # For pip dependencies which have entry points, the `entry_point` macro can be
-# used from the generated `pip_install` repository to access a runnable binary.
+# used from the generated `pip_parse` repository to access a runnable binary.


Dont think this should change here. This is pip_install/BUILD

hrfuller · 2021-10-18T22:56:02Z

 )

-http_archive(
+local_repository(


hrfuller · 2021-10-18T22:57:43Z

 )

-# Test the use of all pip_parse utilities in a single py_test
+# Test the use of all pip_install utilities in a single py_test


This and the other comment changing the rule name seem wrong. Am I missing something?

hrfuller · 2021-10-18T22:57:56Z

 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

-http_archive(
+local_repository(


hrfuller · 2021-10-18T22:58:07Z

 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

-http_archive(
+local_repository(


hrfuller · 2021-10-18T23:01:19Z

        if __name__ == "__main__":
-            from {module} import {method}
-            {method}()
+            sys.exit({method}())


I wonder how often the convention of returning a numerical exit code holds for scripts in PyPI.

@gibfahn What I mean is that we should actually check that this is an int / or at least coercible to an int before calling sys.exit.

Sorry, missed replying to this one. Was going to say I considered scanning pypi for packages that return an int, but there didn't seem to be a straightforward way of checking that without some fancy mypy style magic 😁

What I mean is that we should actually check that this is an int / or at least coercible to an int before calling sys.exit.

Yeah okay, let me take a look.

So looking at other similar things, existing Python tooling does not check. I am not sure we can check without a lot of code additions.

isn't it just is_integer as a runtime check?

>>> 1.5.is_integer() False >>> 1.0.is_integer() True >>> 1.4142135623730951.is_integer() False

it's much better to exit 0 then to throw, if some module has a non-int main return value. And someone surely does

Okay, added that check, please take a look.

Pip doesn't seem to do this, so I'd say chances are probably low that it's in use, but better safe than sorry.

alexeagle · 2021-10-25T22:11:57Z

+
 alias(
    name = "yamllint",
+    # If `pkg` and `script` are the same, passing a single string to


this is confusing, there's only one thing below.
do you mean "since pkg and script are the same"?

Yep, this is wrong, fixed.

alexeagle · 2021-10-25T22:13:03Z


+alias(
+    name = "yamllint",
+    # If `pkg` and `script` are the same, passing a single string to


again, comment doesn't align with code

alexeagle · 2021-10-25T22:14:59Z

        if __name__ == "__main__":
-            from {module} import {method}
-            {method}()
+            sys.exit({method}())


isn't it just is_integer as a runtime check?

>>> 1.5.is_integer() False >>> 1.0.is_integer() True >>> 1.4142135623730951.is_integer() False

it's much better to exit 0 then to throw, if some module has a non-int main return value. And someone surely does

hrfuller

Thanks for the changes. LGTM.

gibfahn requested review from brandjon and lberki as code owners October 13, 2021 11:15

google-cla Bot added the cla: yes label Oct 13, 2021

gibfahn force-pushed the entry-point-return branch 2 times, most recently from d818b19 to 45ce4d0 Compare October 14, 2021 11:27

gibfahn requested review from andyscott and thundergolfer as code owners October 14, 2021 11:27

google-cla Bot added cla: no and removed cla: yes labels Oct 14, 2021

gibfahn force-pushed the entry-point-return branch from 45ce4d0 to cbe7c1a Compare October 14, 2021 11:29

google-cla Bot added cla: yes and removed cla: no labels Oct 14, 2021

hrfuller reviewed Oct 18, 2021

View reviewed changes

gibfahn force-pushed the entry-point-return branch from cbe7c1a to 7284876 Compare October 20, 2021 19:52

alexeagle reviewed Oct 25, 2021

View reviewed changes

pip: entry_point: Add support for exit codes

ed55f1e

gibfahn force-pushed the entry-point-return branch from 7284876 to ed55f1e Compare November 5, 2021 21:52

Merge branch 'main' into entry-point-return

097c0b6

hrfuller approved these changes Nov 10, 2021

View reviewed changes

hrfuller merged commit 431caac into bazel-contrib:main Nov 10, 2021

gibfahn deleted the entry-point-return branch November 11, 2021 17:53

Uh oh!

Uh oh!

Conversation

gibfahn commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Commits (oldest to newest)

Uh oh!

google-cla Bot commented Oct 14, 2021

Uh oh!

UebelAndre commented Oct 18, 2021

Uh oh!

hrfuller left a comment

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!

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!

hrfuller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gibfahn commented Oct 13, 2021 •

edited

Loading