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

Configure tox #1667

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Configure tox #1667

merged 4 commits into from
Sep 19, 2023

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Sep 19, 2023

This adds a tox.ini file so that tox can be run locally to test on 3.7, 3.8, 3.9, 3.10, 3.11, and 3.12, or whichever of them are available, and also run the lint, formatting, and style checks.

Because CI also tests building the documentation, and it is convenient to be able to use tox to check all the things CI checks (albeit on just one's local platform), I have added a tox environment for that too. But because it actually writes to the build directory, I have not listed it in env_list at the top, so it doesn't run automatically--it can still be run with tox -e html. That could be improved in the future, but I think reasonable ways of doing so would involve modifying doc/Makefile to make it more configurable, and that seems outside the proper scope of this PR.

This PR does not use tox on CI. It is possible to use tox to drive tests on CI, such as with the tox-gh-actions plugin, but this does not do that, because:

  • I feel it would be outside the scope of this PR, because the benefit of tox for this project is not primarily for what it brings to CI nor even for making local and CI based testing more consistent.
  • If it is to be done, I think it may be better to do it after native Windows tests are added, which may have bearing on how (and maybe even if) it should be done.
  • Most importantly, migrating from a more traditional GitHub Actions workflow for testing to a tox workflow will sometimes make results harder to read, because they are no longer divided into top-level steps belonging to the test job and clearly shown as separate. This doesn't mean it shouldn't be done, and either manual customization or alternatives to tox-gh-actions such as tox-gh-matrix might help.
  • In my view, the biggest gains from using tox on CI are for when the tox environments are more granular than the CI jobs. This happens in projects that need to parameterize their test runs not just on OS and Python version but also on dependency versions (while avoiding the proliferation of separate CI jobs), which I don't think is applicable here.

So while I suspect it may eventually be a good idea to use tox on CI, I don't think it should be done until it is clearly no worse than the current approach and other readily available approaches.

My intention is also not for tox to replace other ways of running tests when developing locally. During routine development, tox may be too slow or its output too complicated, for regularly checking the status of some or all tests. Furthermore, pre-commit seems to be working great for the lint checks--and the lint tox environment I configured uses it, to as to pick up any new or changed linting tools automatically. (Also, not everyone likes tox.)

For these reasons, I have not modified the readme to replace anything with tox-based instructions. However, the readme could be modified to also talk about tox. I am not sure if that should be done either, because the users most interested in using tox are those already familiar with it, and they will notice the tox.ini. Furthermore, I think it may be best to see how it works out (for example, should it really have the environment for testing that we can build docs by actually building them?) before adding it to the readme or other project documentation. However, if you'd like tox instructions, or a mention of it, etc., added the readme, I'd be pleased to add it (as well as to make other requested changes).

Some other considerations:

  • The one problem I encountered initially in using tox to run tests was being prompted to enter my passphrase for some tests. Because that completely breaks the useful automation tox provides, I allowed tox to pass environment variables whose names start with SSH_ through to its test environments. It would probably be sufficient to pass through SSH_AGENT_PID and SSH_AUTH_SOCK; I'd be pleased to change it to just that, if requested. (If other forms of authentication than SSH that I'm not using rely on environment variables being set, then that might still be broken.)
  • Because the version of Python being tested on CI for Cygwin is 3.9, which is also the latest version available through the Cygwin package manager, the tox environments for everything other than the tests are specified as 3.9, to support Cygwin and to allow results to be compared between Cygwin and other platforms. Also, because Cygwin doesn't have 3.10, 3.11, or 3.12, I have not specified skip_missing_interpreters = true, leaving it to the default of false. Nonetheless, I do not claim tox behaves perfectly on Cygwin in this configuration. I sometimes have problems where it tries to use Python interpreters from my native system before eventually finding something it can really use.
  • I've removed tox from requirements-dev.txt, and not added it to test-requirements.txt since it should rarely be installed together with other project dependencies in the same environment. It's possible to install tox in a virtual environment that one is using for the project, but this is not typically done, because tox creates its own environments. It's best to install tox with pipx when possible. Furthermore, if any version of tox is available, and that version is lower than the version specified in tox.ini, then tox will bootstrap a virtual environment and install a newer version of itself in it. Of course, one can still run pip install tox in a virtual environment (or the global environment) if one wishes to get tox that way.
  • I considered putting the tox configuration in pyproject.toml to avoid adding yet another top-level file. But I decided against it. It forgoes the discoverability benefits noted above in my reasoning about why I haven't mentioned tox in the readme. Also, it has to be given as one giant string, and editors (and GitHub) don't currently special-case that with useful syntax highlighting.

It is not completely working yet.
This fixes a problem where the tests for fetching a nonexistent ref
prompt like "Enter passphrase for key '/home/USERNAME/.ssh/id_rsa':"
and block, if the repository on the machine where the tests are
being run has the remote set up using an SSH URL.

This passes through all environment variables whose names start
with SSH_, even though it should be enough to pass SSH_AGENT_PID
and SSH_AUTH_SOCK through, at least for this particular issue.
Other environments would still be run even after mypy has failed,
but to avoid having tox runs be unnecessarily inconsistent with the
mypy step in the pythonpackage.yml CI workflow, and also because
GitPython is not currently expected to pass mypy checks, this keeps
mypy errors from causing the whole tox run to be reported as
failed.
The main use of this, similar to the step at the end of
pythonpackage.yml, is to find errors produced by building.

However, actual documentation *is* built, and unlike other tox
environments, running this one actually writes outside the .tox/
directory, creating the documentation in the usual target
location. For that reason, this environment is omitted from the
env_list, so that it does not run by default and unexpectedly
overwrite documentation that may recently have been built before
changes are made that could cause generated documentation to be
different.
@EliahKagan EliahKagan marked this pull request as ready for review September 19, 2023 05:10
@Byron Byron added this to the v3.1.37 - Bugfixes milestone Sep 19, 2023
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a million for all of your great work!

I remember having had tox.ini for a while, but don't recall why it was removed.

Furthermore, I follow the reasoning behind this PR and agree that at this point it's probably not worth mentioning it directly in the README. The simplest way to run tests, albeit only in the current interpreter, still seems to be to use venv. I also think it's valid to let CI test everything. But what happens if a test fails in a python version that one doesn't have locally? Then having tox would certainly help fixing it.

Maybe in follow-up PR the contribution guide could be adjusted to include tox related development workflows, along with hints about why one would use it. That way it's presented as more of an evolution rather than a requirement. Also, I haven't heard of an alternative so tox seems to have uniquely useful capabilities, whether one likes its output style or not.

Regarding tox on CI, I'd definitely prefer keeping CI as is. Using tox related workflows certainly has advantages when starting a project as it saves dealing with GHA oneself, but since we have that already, using tox-specific workflow seems to abstract and occlude. Or in other words, right now I'd feel more in control with GHA, it's the devil I know.

tox.ini Show resolved Hide resolved
@Byron Byron merged commit 1256b16 into gitpython-developers:main Sep 19, 2023
@EliahKagan EliahKagan deleted the tox branch September 19, 2023 08:00
@EliahKagan
Copy link
Contributor Author

Also, I haven't heard of an alternative so tox seems to have uniquely useful capabilities, whether one likes its output style or not.

There is also nox, which I have heard is even more powerful, but I haven't really used it.

@Byron
Copy link
Member

Byron commented Sep 20, 2023

Right! Since tox is the most used one, I think it's probably the one to keep unless there are issues that nox would solve.

renovate bot referenced this pull request in allenporter/flux-local Sep 23, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [GitPython](https://togithub.com/gitpython-developers/GitPython) |
`==3.1.36` -> `==3.1.37` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.36/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.36/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.37`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.37):
- a proper fix CVE-2023-41040

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.36...3.1.37)

#### What's Changed

- Improve Python version and OS compatibility, fixing deprecations by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1654](https://togithub.com/gitpython-developers/GitPython/pull/1654)
- Better document env_case test/fixture and cwd by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1657](https://togithub.com/gitpython-developers/GitPython/pull/1657)
- Remove spurious executable permissions by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1658](https://togithub.com/gitpython-developers/GitPython/pull/1658)
- Fix up checks in Makefile and make them portable by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1661](https://togithub.com/gitpython-developers/GitPython/pull/1661)
- Fix URLs that were redirecting to another license by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1662](https://togithub.com/gitpython-developers/GitPython/pull/1662)
- Assorted small fixes/improvements to root dir docs by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1663](https://togithub.com/gitpython-developers/GitPython/pull/1663)
- Use venv instead of virtualenv in test_installation by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1664](https://togithub.com/gitpython-developers/GitPython/pull/1664)
- Omit py_modules in setup by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1665](https://togithub.com/gitpython-developers/GitPython/pull/1665)
- Don't track code coverage temporary files by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1666](https://togithub.com/gitpython-developers/GitPython/pull/1666)
- Configure tox by [@&#8203;EliahKagan](https://togithub.com/EliahKagan)
in
[https://github.com/gitpython-developers/GitPython/pull/1667](https://togithub.com/gitpython-developers/GitPython/pull/1667)
- Format tests with black and auto-exclude untracked paths by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1668](https://togithub.com/gitpython-developers/GitPython/pull/1668)
- Upgrade and broaden flake8, fixing style problems and bugs by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1673](https://togithub.com/gitpython-developers/GitPython/pull/1673)
- Fix rollback bug in SymbolicReference.set_reference by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1675](https://togithub.com/gitpython-developers/GitPython/pull/1675)
- Remove `@NoEffect` annotations by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1677](https://togithub.com/gitpython-developers/GitPython/pull/1677)
- Add more checks for the validity of refnames by
[@&#8203;facutuesca](https://togithub.com/facutuesca) in
[https://github.com/gitpython-developers/GitPython/pull/1672](https://togithub.com/gitpython-developers/GitPython/pull/1672)

**Full Changelog**:
gitpython-developers/GitPython@3.1.36...3.1.37

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@EliahKagan
Copy link
Contributor Author

I think nox could probably take the place of both make and tox. But even if so, I'm not sure if that's really enough reason to switch to it, for this project.

@Byron
Copy link
Member

Byron commented Sep 27, 2023

To my mind, the less python is needed, the better, so I am happy to stay with the little Makefile used here 😅.
If nox has any edge over tox that would solve a problem, then I'd consider this an incentive though - you would be the judge of that.

otc-zuul bot pushed a commit to opentelekomcloud-infra/eyes_on_docs that referenced this pull request Oct 25, 2023
Bump gitpython from 3.1.35 to 3.1.37

Bumps gitpython from 3.1.35 to 3.1.37.

Release notes
Sourced from gitpython's releases.

3.1.37 - a proper fix CVE-2023-41040
What's Changed

Improve Python version and OS compatibility, fixing deprecations by @​EliahKagan in gitpython-developers/GitPython#1654
Better document env_case test/fixture and cwd by @​EliahKagan in gitpython-developers/GitPython#1657
Remove spurious executable permissions by @​EliahKagan in gitpython-developers/GitPython#1658
Fix up checks in Makefile and make them portable by @​EliahKagan in gitpython-developers/GitPython#1661
Fix URLs that were redirecting to another license by @​EliahKagan in gitpython-developers/GitPython#1662
Assorted small fixes/improvements to root dir docs by @​EliahKagan in gitpython-developers/GitPython#1663
Use venv instead of virtualenv in test_installation by @​EliahKagan in gitpython-developers/GitPython#1664
Omit py_modules in setup by @​EliahKagan in gitpython-developers/GitPython#1665
Don't track code coverage temporary files by @​EliahKagan in gitpython-developers/GitPython#1666
Configure tox by @​EliahKagan in gitpython-developers/GitPython#1667
Format tests with black and auto-exclude untracked paths by @​EliahKagan in gitpython-developers/GitPython#1668
Upgrade and broaden flake8, fixing style problems and bugs by @​EliahKagan in gitpython-developers/GitPython#1673
Fix rollback bug in SymbolicReference.set_reference by @​EliahKagan in gitpython-developers/GitPython#1675
Remove @NoEffect annotations by @​EliahKagan in gitpython-developers/GitPython#1677
Add more checks for the validity of refnames by @​facutuesca in gitpython-developers/GitPython#1672

Full Changelog: gitpython-developers/[email protected]



Commits

b27a89f fix makefile to compare commit hashes only
0bd2890 prepare next release
832b6ee remove unnecessary list comprehension to fix CI
e98f57b Merge pull request #1672 from trail-of-forks/robust-refname-checks
1774f1e Merge pull request #1677 from EliahKagan/no-noeffect
a4701a0 Remove @NoEffect annotations
d40320b Merge pull request #1675 from EliahKagan/rollback
d1c1f31 Merge pull request #1673 from EliahKagan/flake8
e480985 Tweak rollback logic in log.to_file
ff84b26 Refactor try-finally cleanup in git/
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the Security Alerts page.

Reviewed-by: Vladimir Vshivkov
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Mar 12, 2024
This is to make it so simple `tox` usage has the expected property
of leaving all source code files in the working tree unchanged.

As noted in c66257e, linting how sometimes performs auto-fixes
since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also
run `black --check`, will do even more file editing with gitpython-developers#1865.

The bifurcation for black into separate mutating and non-mutating
hooks, introduced in 5d8ddd9 (gitpython-developers#1693), is not carried over into Ruff
autoformatting in gitpython-developers#1865. But also it:

- Was not necessarily a good approach, and likely should not be
  preserved in any form. It is an unusual and unintuitive use of
  pre-commit. (It can be brought back if no better approach is
  found, though.)

- Was done to avoid a situation where it was nontrivial to set up
  necessary dependencies for linting in the GitPython virtual
  environment itself, because flake8 and its various plugins would
  have to be installed.

  They were not listed in any existing or newly introduced extra
  (for example, they were not added to test-requirements.txt) in
  part in the hope that they would all be replaced by Ruff, which
  happened in gitpython-developers#1862.

- Already does not achieve its goal since gitpython-developers#1862, since it was
  (probably rightly) not extended to Ruff linting to use/omit --fix.

Now that Ruff is being used, people can run `pip install ruff` in a
virtual environment, then run the `ruff` command however they like.
This takes the place of multiple tools and plugins.

This commit *avoids* doing any of the following, even though it may
be useful to do them later:

- This does not give specific instructions in the readme for
  installing and running ruff (and c66257e before this also omits
  that). This can be added later and the best way to document it
  may depend on some other upcoming decisions (see below).

- This does not add ruff to the test extra or as any other kind of
  extra or optional dependency. Although the test extra currently
  contains some packages not used for running unit tests, such as
  pre-commit and mypy, adding Ruff will cause installation to take
  a long time and/or or fail on some platforms like Cygwin where
  Ruff has to be built from (Rust) source code. This can be solved
  properly by reorganizing the extras, but that is likely to wait
  until they are expressed completely inside pyproject.toml rather
  than one per requirements file (see discussion in comments
  in gitpython-developers#1716 for general information about possible forthcoming
  changes in how the project is defined).

- This does not update tox.ini to run ruff directly, which could be
  done by splitting the current lint tox environment into two or
  three environments for Python linting, Python autoformatting, and
  the miscellaneous other tasks performed by pre-commit hooks, only
  the latter of which would use the pre-commit command. Whether and
  how this should be done may depend on other forthcoming changes.

- This does not remove or update the Makefile "lint" target. That
  target should perhaps not have been added, and was always meant
  to be improved/replaced, since everything else in the top-level
  Makefile is for building and publishing releases. See 5d15063
  (gitpython-developers#1693). This is likewise not done now since it may depend on
  as-yet unmerged changes and tooling decisions not yet made. It
  should be feasible to do together when further updating tox.ini.

- This does not update tox.ini, Makefile, or the lint.yml GitHub
  Actions workflow to omit the manual hook-stage, which will be
  unused as of gitpython-developers#1865. This would complicate integration of changes,
  and once it is known that it won't be needed, it can removed.

The situation with the tox "lint" environment is thus now similar
to that of the tox "html" environment when it was added in e6ec6c8
(gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with
proper isolation.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Mar 13, 2024
This is to make it so simple `tox` usage has the expected property
of leaving all source code files in the working tree unchanged.

Linting how sometimes performs auto-fixes since gitpython-developers#1862, and the
pre-commit command in tox.ini, which had also run `black --check`,
will do even more file editing due to the changes in gitpython-developers#1865.

The bifurcation for black into separate mutating and non-mutating
hooks, introduced in 5d8ddd9 (gitpython-developers#1693), was not carried over into
Ruff autoformatting in gitpython-developers#1865. But also it:

- Was not necessarily a good approach, and likely should not be
  preserved in any form. It was an unusual and unintuitive use of
  pre-commit. (It can be brought back if no better approach is
  found, though.)

- Was done to avoid a situation where it was nontrivial to set up
  necessary dependencies for linting in the GitPython virtual
  environment itself, because flake8 and its various plugins would
  have to be installed.

  They were not listed in any existing or newly introduced extra
  (for example, they were not added to test-requirements.txt) in
  part in the hope that they would all be replaced by Ruff, which
  happened in gitpython-developers#1862.

- Already did not achieve its goal as of gitpython-developers#1862, since it was
  (probably rightly) not extended to Ruff linting to use/omit --fix.

Now that Ruff is being used, people can run `pip install ruff` in a
virtual environment, then run the `ruff` command however they like.
This takes the place of multiple tools and plugins.

The situation with the tox "lint" environment is thus now similar
to that of the tox "html" environment when it was added in e6ec6c8
(gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with
proper isolation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants