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

Port `ci.sh` to Python 3 for more descriptive CLI flags and less duplication #7849

Merged
merged 16 commits into from Jun 6, 2019

Conversation

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

commented Jun 5, 2019

Problem

.travis.yml can be cryptic to read due to ci.sh's singular use of short CLI flags. For example, it is difficult to explain what this line does without checking the script or surrounding context:

./build-support/bin/ci.sh -fmrt2

Solution

Port the script to modern Python 3, which allows us to use argparse.

Further, we can now better express choosing the Python version to use. Rather than having the flags -2 and -7, now you specify --python-version {2.7,3.6,3.7} or -v {2.7,3.6,3.7}. You may only choose one, and it defaults to 3.6.

Finally, using Python 3 allows us to improve the code itself, such as de-duplicating sections and avoiding materializing targets to disk with the run_python_tests() section.

Result

CI will run the same, but will hopefully be easier to understand and to iterate on. For example, the above command becomes:

./build-support/bin/ci.py --githooks --sanity-checks --doc-gen --lint --python-version 2.7

Eric-Arellano added some commits Jun 5, 2019

Squashed commit of the following:
commit 34596b5
Merge: b16b158 bf7263d
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 17:38:36 2019 -0700

    Merge branch 'master' of github.com:pantsbuild/pants into merge-contrib-ci

commit b16b158
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 17:38:24 2019 -0700

    Review feedback

commit 279ea5a
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 08:40:19 2019 -0700

    Run internal tests separately from unit tests

commit b5569cc
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 08:37:39 2019 -0700

    Break out contrib integration test into its own process

    Makes for a faster failure if the core integration tests fail

commit 9d5a2fd
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 08:33:52 2019 -0700

    Run contrib unit tests

commit f7a9540
Merge: f37db31 e2b18b9
Author: Eric Arellano <ericarellano@me.com>
Date:   Tue Jun 4 08:00:03 2019 -0700

    Merge branch 'master' of github.com:pantsbuild/pants into merge-contrib-ci

commit f37db31
Author: Eric Arellano <ericarellano@me.com>
Date:   Mon May 13 09:58:22 2019 -0700

    Exclude testprojects

    The contrib folders have these, but we don't want to run them.

commit ef146e6
Author: Eric Arellano <ericarellano@me.com>
Date:   Mon May 13 08:10:19 2019 -0700

    Run contrib tests with unit tests and integration tests for less CI delays
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Hey all, pardon that this is a big review.

Here's how I recommend reviewing it:

  1. Start with https://github.com/pantsbuild/pants/pull/7849/files#diff-88af3146f5cc486b749ed790399bde46 to see the result of these changes and the reason I did this all.
  2. Pull up https://github.com/pantsbuild/pants/pull/7849/files#diff-87d2a65a0562160945633338037cec9b on half your screen and https://github.com/pantsbuild/pants/pull/7849/files#diff-41af3376739df0aa678f6e005f62dcac on the other half. They are closely similar, including in file organization. Below are the differences. Everything else, including the short CLI flags, is the same.
    • Python does not implement the $REPO_ROOT logic. I could not figure out quickly how to do it. If necessary, can add.
    • Changed the API for Python version to be a choice from an enum. See the PR description.
    • Tweaked some of the travis_section and die messages. Nothing major.
    • Python's run_python_tests() no longer saves targets to files. Keeps in memory.
  3. Review the other files, which are just doc / comment changes.
@jsirois

jsirois approved these changes Jun 5, 2019

Copy link
Member

left a comment

Thanks for doing this.

Show resolved Hide resolved build-support/bin/ci.py
Show resolved Hide resolved build-support/bin/ci.py Outdated

Eric-Arellano added some commits Jun 5, 2019

Add eager evaluation that pants.pex exists
Provides a better UX.
Drop short flags
There isn't a very strong case for keeping them. Removing them brings these benefits:

* More helpful `./ci.py --help` message.
* Less code.
* Less context switching, e.g. a Pants dev sending in Slack the short flag name and then other devs having to figure out what it means.
* Easier to introduce new flags, without worrying that the short name is already taken.
@jsirois

jsirois approved these changes Jun 5, 2019

@benjyw

benjyw approved these changes Jun 5, 2019

Copy link
Contributor

left a comment

I am also in favor of scrapping the short flags entirely, as you've done. If we didn't have the legacy of short flags in the shell script, we wouldn't have thought to have them here.

@Eric-Arellano Eric-Arellano referenced this pull request Jun 5, 2019

Merged

Apply isort fixes #7857

Eric-Arellano added a commit that referenced this pull request Jun 5, 2019

Apply isort fixes (#7857)
In #7849, the lint shard was failing due to isort complaining about a file whose comment had to be updated.

When running `./pants --cache-ignore fmt.isort tests/python:: src/python:: build-support/bin:: pants-plugins::` on master, indeed several files have issues. They were not caught because `isort.sh` only runs against changed files.

Eric-Arellano added some commits Jun 5, 2019

Fix sanity checks
Was failing because `list ::` must be treated as a list, not one single string.
Only check for pants.pex if test actually needs it
For example, Clippy never uses pants.pex, so it doesn't make sense to require it.

We keep the check, however, because it is a much nicer user experience to get the eager die() message that instructs users how to fix it, rather than trying to figure out on their own how to fix things.

@Eric-Arellano Eric-Arellano merged commit 256b696 into pantsbuild:master Jun 6, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:ci-py branch Jun 6, 2019

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.