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.

Sign up for GitHub

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

Feature/pep517 release #226

Merged
merged 3 commits into from
Sep 26, 2019
Merged

Feature/pep517 release #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7287089
Add tox release relying on pep517.
jaraco Sep 26, 2019
43ec070
Rely on tox -e release during mechanical releases.
jaraco Sep 26, 2019
31150f6
Rely on environment variable to indicate TOXENV
jaraco Sep 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,10 @@ jobs:
- <<: *stage_deploy
if: tag IS present
<<: *python_3_7_mixture
install: skip
script: skip
deploy:
provider: pypi
on:
all_branches: true
user: jaraco
password:
secure: RAfz06AINvz7bfij/YhfkAreRqamgxS8a6jSRNxntYhtJke3ZszUbIDag8+n1I+G5XT2LnMhHqPNR7Plc+AeMz7VXTuy+b81Li5kse20NYlPhd7mBVmTUpXtqYQashV5J39F4qkATBLznCOrMEomM07VTXjO/o2hmQuXniab2Uo=
distributions: dists
skip_cleanup: true
install:
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, just add tox-venv to tox.ini and set

Suggested change
install:
env:
TOXENV: release

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I want to invoke tox -m tox --notest or *python_ssl_openssl_version or python -m OpenSSL.debug during install, which is why I chose to override it.

Also, I'm not sure about adding tox-venv to tox.ini. There may be bootstrapping issues. If tox needs a later version or needs to install tox-venv, then it will create a .tox/.tox env in which to install those packages. If tox-venv isn't already installed (or is), that may affect how .tox/.tox is created. The only technique I trust is to install tox-venv alongside tox prior to invoking .tox. I'm open to experimenting with other techniques, but I don't want to adopt those risks as part of this change.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so let's change this to installing tox-venv and invoking tox with --notest because --notest allows us to clearly separate envs bootstrap (hence the install step) from actually running main commands (script).
Using env allows us to unify interactions with tox across the config file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. By running --notest, you get bootstrapping of the environment(s) before running their commands. That's interesting and does make sense. I'm reluctant to add complication, but I can see the benefit there. Let's explore that after validating this simpler approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that was my point :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this discussion got distributed over comments... #226 (comment)

- python -m pip install tox tox-venv
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- python -m pip install tox tox-venv
- python -m pip install tox tox-venv
- python -m tox --notest

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks but no thanks. I'm not going to add a command and complication when the default process does the same thing. Better to rely on tox to do what it does best, rather than try to micro-manage it. This change seems counter to best-practices... and I can hardly imagine the benefit.

Copy link
Member

Choose a reason for hiding this comment

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

I consider it best practice. Seen it used in a lot of places. tox itself uses --notest as it was designed for such separation: https://github.com/tox-dev/azure-pipelines-template/blob/master/run-tox-env.yml#L118

env:
TOXENV: release

cache:
pip: true
Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
[build-system]
requires = ["setuptools>=30.3", "wheel", "setuptools_scm>=1.15"]
requires = ["setuptools>=34.4", "wheel", "setuptools_scm>=1.15"]
build-backend = "setuptools.build_meta"
15 changes: 15 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,18 @@ commands =
python -m setup checkdocs check --metadata --restructuredtext --strict --verbose
python -m twine check .tox/dist/*
python -m setuptools_scm ls

[testenv:release]
skip_install = True
deps =
pep517>=0.5
twine>=1.13
path.py
passenv =
TWINE_PASSWORD
setenv =
TWINE_USERNAME = {env:TWINE_USERNAME:__token__}
commands =
python -c "import path; path.Path('dist').rmtree_p()"
python -m pep517.build .
Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer having separate build and publish envs. Then we'd do smth like TOXENV=build-dists,setup-check,release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Three steps for one artifact? I dunno. That sounds overly complicated. Then you'd have to ensure that the artifacts from each invocation are shared across the three steps... and a manual release would then require the same steps (or at least the build/release steps). I can imagine a few scenarios where it might be useful to have separate steps, but 99% of the use-cases are going to be to invoke the suite of commands, so I'd prefer to keep it simple. And in that 1% of the cases where it's necessary to separate the concerns, one could use tox --notest and run the commands incrementally. Why not just keep it simple and concise?

Copy link
Member

Choose a reason for hiding this comment

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

I think it does too much. I often want a command that gives me dists what I can then inspect with other tools. How about something like this https://github.com/pypa/twine/blob/5cb1867/tox.ini#L53-L61 then?

Copy link
Member

Choose a reason for hiding this comment

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

Also, ideally in the future, I'd like to try out a better testing strategy: first build the artifacts, then pip-install from there and completely avoid lib imports from the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to try out a better testing strategy

Yes, maybe. I'm heavily invested in the test-at-source methodology because it's faster. It's one of the reasons I prefer Python to (your language here). I do acknowledge there's weakness in testing against the source instead of the built artifact... but my instinct there would be to provide additional testing for the build artifact rather than encumber the development process with two sources of truth (source and installed code).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that for the development purposes it's more comfortable to rely on editable installs. And yes, I do agree that we need more artifact testing in CI than locally. I've been meaning to implement this for a while.
Here's a nice article that Paul published this topic last month: https://blog.ganssle.io/articles/2019/08/test-as-installed.html

python -m twine upload dist/*