Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

Add black and isort #124

Merged
merged 3 commits into from
Jun 24, 2020
Merged

Add black and isort #124

merged 3 commits into from
Jun 24, 2020

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Jun 4, 2020

Resolves #123

Adds the black and isort tools to the development requirements, and adds Makefile targets for both checking and applying them.

Removes the apparently unused Sphinx requirement, which would otherwise need to be upgraded to address a jQuery security problem.

I'm configuring both black and isort in pyproject.toml, in the repo root, because according to PEP 517, "If the pyproject.toml file is absent, or the build-backend key is missing, the source tree is not using this specification, and tools should revert to the legacy behaviour of running setup.py (either directly, or by implicitly invoking the setuptools.build_meta:__legacy__ backend)."

The configuration is using Black's suggested isort settings for compatibility, as well as an additional isort setting that will appear in an upcoming release.

Testing

  • Check out the first commit on black-isort with git checkout 24dc1ea
  • Run make check-isort. It should show the changes it would make.
  • Run make check-black. It should show the changes it would make.
  • Check out the last commit: git checkout black-isort
  • Rerun make check-isort and make check-black. No changes should be indicated.
  • Run git blame sdclientapi/__init__.py | head -21 | grep -c a31be70d. There should be seven lines attributed to that revision.
  • Run:
    • git config blame.ignoreRevsFile .git-blame-ignore-revs
    • git config blame.markIgnoredLines true
    • git config blame.markUnblamableLines true
  • Run git blame sdclientapi/__init__.py | head -21 | grep -c a31be70d. There should no lines attributed to a31be70, now that the formatting revision is being ignored.

@rmol rmol changed the title Black isort Add black and isort Jun 4, 2020
@rmol rmol marked this pull request as ready for review June 4, 2020 21:03
@rmol
Copy link
Contributor Author

rmol commented Jun 4, 2020

I've marked this ready for review. I'm particularly hoping @kushaldas can take a look and weigh in on the pyproject.toml placement, given the errors he saw in freedomofpress/securedrop-proxy#61.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

I would prefer to store the pyproject.toml in a different directory, or else we may again get stuck during packaging time.

@kushaldas kushaldas dismissed their stale review June 5, 2020 09:27

Let it be a comment, not change request.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

All looks good, but the final test step is not working for me. @rmol can you please tell me if I am missing something.

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) took 36s 
❯ git log -1
commit e83ebfe79405fdca80ae5952f413c7cb9866129a (HEAD -> black-isort, origin/black-isort)
Author: John Hensley <john@freedom.press>
Date:   Thu Jun 4 16:17:54 2020 -0400

    Add .git-blame-ignore-revs

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) 
❯ git config blame.ignoreRevsFile .git-blame-ignore-revs

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) 
❯ git config blame.markIgnoredLines true

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) 
❯ git config blame.markUnblamableLines true

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) 
❯ git blame -e sdclientapi/__init__.py | head -21 | grep -c john@freedom.press
7

securedrop-sdk on  black-isort [?] via 🐍 v3.7.3 (.venv) 
❯ git version
git version 2.20.1

@rmol
Copy link
Contributor Author

rmol commented Jun 8, 2020

@kushaldas Yeah, the git blame options to ignore revisions weren't added until git version 2.23.

Can you explain the packaging problems the default location causes? As far as I can see, the problems pip and setuptools had when the presence of pyproject.toml switched them to PEP-517 mode were fixed. It looks like the PyPA projects themselves (at least pip, setuptools, and virtualenv) are now using it -- albeit with build-backend explicitly specified.

I want to avoid requiring extra work for contributors to be able to run our formatting tools so that CI lint jobs pass.

@kushaldas
Copy link
Contributor

@kushaldas Yeah, the git blame options to ignore revisions weren't added until git version 2.23.

Ah yes, I wrote the other patch in a different box :)

Can you explain the packaging problems the default location causes? As far as I can see, the problems pip and setuptools had when the presence of pyproject.toml switched them to PEP-517 mode were fixed. It looks like the PyPA projects themselves (at least pip, setuptools, and virtualenv) are now using it -- albeit with build-backend explicitly specified.

I want to avoid requiring extra work for contributors to be able to run our formatting tools so that CI lint jobs pass.

dh-virtualbox failed to build for me, but maybe I did not something wrong. Can you please give it a try today? or else I will try tomorrow with these changes and get back to you.

@rmol
Copy link
Contributor Author

rmol commented Jun 8, 2020

@kushaldas I tried adding pyproject.toml to securedrop-client and building it in securedrop-debian-packaging, and did indeed get an error from dh-virtualenv:

ERROR: Command errored out with exit status 1: /home/user/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/bin/python /home/user/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.7/site-packages/pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-4flb400p/overlay --no-warn-script-location -v --no-binary :none: --only-binary :none: -i https://pypi.securedrop.org/simple -- setuptools wheel Check the logs for full command output.

It's looking for setuptools on our PyPI mirror and can't find it, because we're not hosting it. I think this is probably something we should fix in our packaging logic or infrastructure, instead of placing pyproject.toml in a nonstandard location, but I'll dig into it a bit more tomorrow.

@kushaldas
Copy link
Contributor

You will have to modify the scripts to disable the signing checks and test against a local server to verify if things are okay.

@rmol
Copy link
Contributor Author

rmol commented Jun 15, 2020

I set up a local PyPI mirror with fallback to pypi.org, and the client package built successfully, suggesting that we can use the standard pyproject.toml location. We just need to make sure our PyPI contains all the packages required.

@rmol
Copy link
Contributor Author

rmol commented Jun 16, 2020

It turns out that the presence of a pyproject.toml with no build-backend specified causes pip to work in a way that "mimics directly executing setup.py" but is evidently not exactly the same, and that's when it tries to download setuptools and wheel. This mode can be avoided entirely by using pip's --no-use-pep517 option in the dh_virtualenv command. (Or alternatively, pip's --no-build-isolation option, which apparently makes it use the setuptools and wheel already in the build environment set up by dh_virtualenv.)

So we don't need to add anything to our PyPI, we just need to add --no-use-pep517 to the dh_virtualenv commands in the securedrop-debian-packaging rules files.

https://pip.pypa.io/en/stable/reference/pip/#pep-517-and-518-support

rmol added 3 commits June 16, 2020 14:46
Add black and isort to dev requirements. Add Makefile targets for both
checking and applying them, and make those part of "make check".

Remove unused Sphinx requirement.

Add pyproject.toml with configuration for both black and isort.
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

This is good, thank you.

@kushaldas kushaldas merged commit 38b285d into master Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use black and potentially isort in this repo
2 participants