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.

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

Mark build-support Python files as Pants targets to lint build-support #7633

Merged
merged 8 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented Apr 27, 2019

Converting to Pants targets

Now that we have two Python scripts, and may have more scripts in the future, there is value in linting our script code.

By making these two scripts as targets, we can now use ./pants fmt, ./pants lint, and ./pants mypy on the build-support folder for little cost.

The pre-commit check will check ./pants fmt and ./pants lint automatically on the build-support Python files now. It will not do the header check until we explicitly add the folder.

Add common.py

We also add common.py to reduce duplication and simplify writing scripts.

See the NB in that file about why we only use the stdlib for that file.

@Eric-Arellano Eric-Arellano requested review from jsirois and cosmicexplorer Apr 27, 2019

@@ -0,0 +1,72 @@
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 27, 2019

Author Contributor

Note this file is adapted from https://github.com/pantsbuild/setup/blob/gh-pages/build-support/common.py. I removed all the parts that aren't relevant to Pants repo.

Currently we are not using the travis_section() and elapsed_time() functions, but I still included them to make it easier for future scripts to use and because they are already battle-tested. I can remove if need be, but I fear others who might write Python scripts wouldn't think to check the setup repo to see that we already have the code written.

RED = "\033[31m"
BLUE = "\033[34m"
RESET = "\033[0m"
from common import die, green

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 29, 2019

Member

This is definitely starting to get weird. Pants targets to allow linting, but a custom green function implemented as a copy of bash where we use ansicolors for said same in pants itself. And I think , etc... until the conversion to ./pants run ... that you describe. I think the python3 porting is a bit premature and that's behind all this. Slightly bettr imo would be to port to python_binary targets first and then when pants can be python 3 only, do the fun stuff. All that said - you deserve a bit of joy after all the grind, so your call on ordering.

Thanks again for all your py3 work.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 29, 2019

Author Contributor

Slightly bettr imo would be to port to python_binary targets first and then when pants can be python 3 only, do the fun stuff.

Porting Pants itself to Py3 won't have any impact on whether we should implement our own color functions vs. use the colors library. We can today run the scripts either via ./pants run or by directly invoking them, and Py3 compatibility works fine as is, even if we were to link it to our 3rd party deps and/or Pants src code.

The idea with solely depending on stdlib is it gives us the option to run directly, rather than requiring us to use ./pants run. I think this is desirable for two reasons:

  1. Closer alignment with how we run bash scripts. It shouldn't matter whether the build-support script is written in Bash or Python—that's an implementation detail. Requiring the user to use ./pants run for one but not even allowing ./pants run for the other (because Pants doesn't support bash targets) is surprising to me.

  2. Ensures we have no dependencies on Pants, as some of the scripts are (will be) used to lint either the bash scripts or source code. For example, I'll soon be adding a shellcheck.py script. If we only allow that script to be ran via ./pants run, then let's say we have an issue with the ./pants script that shellcheck.py is meant to check, the script won't be able to properly run shellcheck because the ./pants run part will fail.

    • Although, now that I write this out, I'm less convinced this is important to maintain the separation. If ./pants run fails because of an issue with the ./pants script, presumably the error will print and the user will know they need to fix it? The only benefit I could see is if shellcheck would help to diagnose the issue.

--

The duplication of common.sh is intentional. Imo, it should be just as easy to develop a Python script as a bash script, meaning we surface the same util code. There will be tasks that are better for Bash (e.g. installing Pyenv) and better for Python (e.g. check_pants_pex_abi, which started as Bash actually)—it should be well supported for the developer to choose the best language for the task at hand.

--

What are your thoughts on argument #1? I'm willing to use colors if you think #1 isn't as much of a concern as I fear. #2 isn't a huge deal it seems.

--

All that said - you deserve a bit of joy after all the grind

Hehe thanks :)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 29, 2019

Author Contributor

Ah one more reason I wanted to allow us to run Python scripts directly, rather than having to use ./pants run: passthru command line flags get a lot less ergonomic when using ./pants run.

Compare

$ build-support/bin/check_pants_pex_abi.py abi3 cp36m

to

$ ./pants run build-support/bin:check_pants_pex_abi -- abi3 cp36m

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 29, 2019

Member

I guess I don't buy any of the direct-run arguments since we have some checks that only run via pants run, like isort. These used to be scripted out of band. I really see nothing wrong with eating our dogfood here. If a non-human is calling run as is the case for your example, an extra -- matters little.

I think we just don't see eye to eye on these efforts and since you're wiling to put the work in - I'll desist in lodging minor tactical objections to how this work is being ordered.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 29, 2019

Author Contributor

Sounds good - I updated the docstring to better address this conversation and will wait for feedback from others on this topic also. Thanks for bringing this all up.

One clarification.

I really see nothing wrong with eating our dogfood here

I completely agree dogfooding is valid, and often desirable, in these scripts. The sole exception I want to make is for common.py, because that is / will be used by the majority of the Python scripts, and I don't want to impose the 3 restrictions on all of those scripts in order to save some duplication of the colors library.

Individual scripts should certainly use 3rd party dependencies and Pants code, where appropriate. I updated the comment to explain this stdlib-only restriction only applies to common.py.

--

Also worth noting that I don't anticipate common.py growing any larger. I'm actively working to remove the complexity I added to the setup repo's version by converting ci.py into a proper unit test file. I don't anticipate common.py ever having more functionality than what common.sh has. For example, if scripts need functionality from dirutil.py, those individual scripts will just directly depend on Pants.

@Eric-Arellano Eric-Arellano changed the title Mark build-support Python files as Pants targets to allow precommit checks Mark build-support Python files as Pants targets to lint build-support Apr 29, 2019

Eric-Arellano added some commits Apr 29, 2019

Better explain why we only use stdlib in common.py
Enumerate the reasons. Also explain this restriction only applies to this file - other scripts may dogfood as desired.

@Eric-Arellano Eric-Arellano requested review from stuhood and blorente May 9, 2019

@stuhood

stuhood approved these changes May 9, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Might be good to rebase before landing, although I don't have any concrete conflicts in mind.

@Eric-Arellano Eric-Arellano merged commit 5eed2e7 into pantsbuild:master May 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:common-py branch May 10, 2019

@blorente
Copy link
Contributor

left a comment

Cool!

@@ -32,7 +29,7 @@ def main() -> None:
if not parsed_abis.issubset(expected_abis):
die("pants.pex was built with the incorrect ABI. Expected wheels with: {}, found: {}."
.format(' or '.join(sorted(expected_abis)), ', '.join(sorted(parsed_abis))))
success("Success. The pants.pex was built with wheels carrying the expected ABIs: {}."
green("Success. The pants.pex was built with wheels carrying the expected ABIs: {}."

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

This might be super bike-sheddy, but I liked the extra semantics conveyed by this being success, meaning that we don't care about which colour this is printed in, we just have to notify success. I'd keep the success function below, and just call green from it, since imo main shouldn't care about which colour means "success". However, this is a super-nit, and in a script this size it might not even be worth it, so feel free to ignore it.

This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

Or just rename green to success, since die exists.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Author Contributor

I see what you mean and agree, although for consistency with common.sh am going to keep it as green. I want those APIs as similar as possible to make it really easy to develop either a bash script or a Python script.

@@ -16,6 +16,7 @@
import sys
from io import open


This comment has been minimized.

Copy link
@blorente

blorente May 10, 2019

Contributor

Extra space?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Author Contributor

Yes. Required to get ./pants fmt build-support:: working.

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request May 10, 2019

Squashed commit of the following:
commit 0a38b338ba5c3d9dd0bc080fa477e8b74e9850e6
Merge: c0bfc9e 9ca32a9
Author: Eric Arellano <ericarellano@me.com>
Date:   Fri May 10 11:20:51 2019 -0700

    Merge branch 'dwagnerhall/strip-prefix' of https://github.com/twitter/pants into twitter-dwagnerhall/strip-prefix

commit c0bfc9e
Author: Eric Arellano <ericarellano@me.com>
Date:   Fri May 10 10:49:18 2019 -0700

    Output stderr in V2 test rule (pantsbuild#7694)

    ### Problem
    Implements pantsbuild#7388. Swallowing stderr makes debugging very difficult.

    Important clarification: failing tests will write to stdout, meaning that we do already output any message captured by Pytest. Instead, this PR is meant to log issues that occur before Pytest executes, such as issues from Pants.

    ### Solution
    Follow the convention established by pantsbuild#7676 to output `{address} stderr:`, followed by the stderr.

commit 55e5721
Author: Daniel Wagner-Hall <dwagnerhall@twitter.com>
Date:   Fri May 10 17:32:03 2019 +0100

    Remove now-unused Path type (pantsbuild#7701)

    The engine lost Paths from its Snapshots at some point, and we didn't clean up.

commit 9ca32a9
Author: Daniel Wagner-Hall <dwagnerhall@twitter.com>
Date:   Fri May 10 10:59:41 2019 +0100

    Allow Directories to be un-nested

commit 5eed2e7
Author: Eric Arellano <ericarellano@me.com>
Date:   Thu May 9 17:38:03 2019 -0700

    Mark build-support Python files as Pants targets to lint build-support (pantsbuild#7633)

    ### Converting to Pants targets
    Now that we have two Python scripts, and may have more scripts in the future, there is value in linting our script code.

    By making these two scripts as targets, we can now use `./pants fmt`, `./pants lint`, and `./pants mypy` on the build-support folder for little cost.

    The pre-commit check will check `./pants fmt` and `./pants lint` automatically on the `build-support` Python files now. It will not do the header check until we explicitly add the folder.

    ### Add `common.py`
    We also add `common.py` to reduce duplication and simplify writing scripts.

    See the `NB` in that file about why we only use the stdlib for that file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.