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

Revert to supporting simple Python factors #2849

Merged
merged 5 commits into from Jan 11, 2023

Conversation

stephenfin
Copy link
Contributor

@stephenfin stephenfin commented Jan 10, 2023

Instead of parsing factors with virtualenv's PythonSpec.from_string_spec, use our own regex to support the simple factors that have been supported since tox v1.

I have submitted this as a draft since this PR removes the functionality without a deprecation period. tox 4 is only a month old and I've wagered that not many people are relying on this functionality yet. I am, however, happy to rework it so we have a deprecation period instead, though that would require additional changes to fix e.g. #2657 (perhaps a temporary environment option to opt-in to the "new" old behavior?)

Fixes: #2657
Fixes: #2848

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Instead of parsing factors with virtualenv's
'PythonSpec.from_string_spec', use our own regex to support the simple
factors that have been supported since tox v1.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Fixes: tox-dev#2657
Fixes: tox-dev#2848
@stephenfin
Copy link
Contributor Author

stephenfin commented Jan 10, 2023

Other points:

  • There's no technical reason we couldn't support python factors with periods in them (e.g. py3.8) though it was never supported before tox 4 and I don't really get why you'd want this extra complexity
  • We could probably expand the list of interpreter prefixes to include e.g. cpython, ironpython (is that still a thing?), etc. Again though, it feels like cpython and pypy are the only two players in town these days so you'd have to question what this gives you.

@gaborbernat
Copy link
Member

gaborbernat commented Jan 10, 2023

  • There's no technical reason we couldn't support python factors with periods in them (e.g. py3.8) though it was never supported before tox 4 and I don't really get why you'd want this extra complexity

We do want this. This will make Github actions simple to setup because you can use the same spec for loading the python version in github as the env you need to run. Removing the gymanstic needed for tox 3 to map 3.8 to py38 👍 This feature will make CI configs much simpler so I'll need to insist on it. See e.g. https://github.com/tox-dev/tox/blob/main/.github/workflows/check.yml#L20-L25 and then https://github.com/tox-dev/tox/blob/main/.github/workflows/check.yml#L43-L50 we need to do just because tox does not support 3.8 as env name and spec. So yes, tox -e 3.8 should work.

  • We could probably expand the list of interpreter prefixes to include e.g. cpython, ironpython (is that still a thing?), etc. Again though, it feels like cpython and pypy are the only two players in town these days so you'd have to question what this gives you.

Today perhaps. There's also rustpython. I don't want to be the gatekeeper of what's reasonable python implementation. We also have ironpython and JVM pythons too. Most people don't use this, but I want tox to allow using these.

We need to be permissive to make people's lives easier, even if they are not in majority.

@stephenfin
Copy link
Contributor Author

  • There's no technical reason we couldn't support python factors with periods in them (e.g. py3.8) though it was never supported before tox 4 and I don't really get why you'd want this extra complexity

We do want this. This will make Github actions simple to setup because you can use the same spec for loading the python version in github as the env you need to run. Removing the gymanstic needed for tox 3 to map 3.8 to py38 +1 This feature will make CI configs much simpler so I'll need to insist on it. See e.g. main/.github/workflows/check.yml#L20-L25 and then main/.github/workflows/check.yml#L43-L50 we need to do just because tox does not support 3.8 as env name and spec. So yes, tox -e 3.8 should work.

Okay, so we need to incorporate periods into the env names. Given it doesn't break the hyphens as delimiters idea, I could live with that.

  • We could probably expand the list of interpreter prefixes to include e.g. cpython, ironpython (is that still a thing?), etc. Again though, it feels like cpython and pypy are the only two players in town these days so you'd have to question what this gives you.

Today perhaps. There's also rustpython. I don't want to be the gatekeeper of what's reasonable python implementation.

packaging is already gatekeeping via INTERPRETER_SHORT_NAMES from packaging.tags so I don't think this is all that relevant?

(BTW, rustpython isn't listed in that dict so you might need to add that?)

We also have ironpython and JVM pythons too. Most people don't use this, but I want tox to allow using these.

We need to be permissive to make people's lives easier, even if they are not in majority.

So we can't use a wildcard because that would falsely identify e.g. django32 or django3.2, which clearly isn't a Python version. I also don't want to rely on INTERPRETER_SHORT_NAMES to build the regex in case it's ever modified to include characters that would lead to an invalid regex (unlikely but possible) or in other ways that could break tox (like removing py). I guess I could expand our hardcoded list to include these additional interpreters, but it seems silly to do this when we could simply document that these users (who are going to be in a minority of a minority) should simply use pyXY factors and rely on base_python to support other interpreters, e.g.

[testenv:py38]
base_python = rustpython3.8

We could also introduce a top-level or testenv-level python_interpreter setting, e.g.

[tox]
python_interpreter = rustpython

[testenv]
commands = ...

Would this be a reasonable compromise: support py, pypy and (purely for legacy reasons) jython but insist on base_python for anything else? We can always expand this regex if a particular interpreter gains enough of a following to warrant first-class support in the future.

@stephenfin
Copy link
Contributor Author

stephenfin commented Jan 11, 2023

  • There's no technical reason we couldn't support python factors with periods in them (e.g. py3.8) though it was never supported before tox 4 and I don't really get why you'd want this extra complexity

We do want this. This will make Github actions simple to setup because you can use the same spec for loading the python version in github as the env you need to run. Removing the gymanstic needed for tox 3 to map 3.8 to py38 +1 This feature will make CI configs much simpler so I'll need to insist on it. See e.g. main/.github/workflows/check.yml#L20-L25 and then main/.github/workflows/check.yml#L43-L50 we need to do just because tox does not support 3.8 as env name and spec. So yes, tox -e 3.8 should work.

Okay, so we need to incorporate periods into the env names. Given it doesn't break the hyphens as delimiters idea, I could live with that.

Oh, wait, you're arguing for tox -e 3.8 instead of tox -e py3.8. That's going to be really error prone. The py prefix (or pypy or jython prefixes) makes this unambiguous and unlikely to conflict with other factors. We can't say the same for something as arbitrary as XY. What are the expected outcomes for these, for example?

  • tox -e 2000
  • tox -e 4.foo
  • tox -e 123
  • tox -e django-23
  • tox -e py-310
  • tox -e catch-22
  • tox -e eslint-8.3

It's also unnecessary. For one, we could easily do this via a plugin like the old tox-travis plugin. A big part of the tox 4 rewrite (as I understand it) was to improve the plugin architecture: surely the use of plugins should be encouraged? You don't need the plugin though. All I've been doing in my CI jobs is running tox -e py. The py environment is special since it implies base_python is whatever sys.executable points to which, as we all know, is determined by the version tox is running (and was installed) under. Install tox under the correct Python version and it'll happily do its job:

[tox]
testenv = py, linters, coverage, docs, typing
jobs:
  test:
    runs-on: ubuntu-22.04
    strategy:
      matrix:
        py:
          - "3.11"
          - "3.10"
          - "3.9"
          - "3.8"
          - "3.7"
    steps:
      - name: Setup Python
        uses: actions/setup-python@v4
        with:
          python-version: ${{ matrix.py }}
      - name: Install tox
        run: python -m pip install tox
      - name: Run tests
        run: tox

Is this somehow insufficient? I mean, couldn't we document this as our recommended practice?

@stephenfin
Copy link
Contributor Author

stephenfin commented Jan 11, 2023

  • There's no technical reason we couldn't support python factors with periods in them (e.g. py3.8) though it was never supported before tox 4 and I don't really get why you'd want this extra complexity

We do want this. This will make Github actions simple to setup because you can use the same spec for loading the python version in github as the env you need to run. Removing the gymanstic needed for tox 3 to map 3.8 to py38 +1 This feature will make CI configs much simpler so I'll need to insist on it. See e.g. main/.github/workflows/check.yml#L20-L25 and then main/.github/workflows/check.yml#L43-L50 we need to do just because tox does not support 3.8 as env name and spec. So yes, tox -e 3.8 should work.

Another point on this: this only works if you're using simple, single-factor testenvs. It doesn't work when you start taking advantage of combinatorial factors, which is an first class feature of tox.

[testenv:py{38,39,310}-django{40,41}]
commands = ...

The https://github.com/ymyzk/tox-gh-actions project already exists and supports tox 4 (I'm using it in a few projects of my own, funnily enough). It's far more powerful than simple support for e.g. 3.8 factors. Perhaps we could reach out to the author and ask if they'd like to move it to the https://github.com/tox-dev organisation, making it a "blessed" plugin (similar to https://github.com/pypa/setuptools_scm in the https://github.com/pypa organisation)? We could also just link to it from the docs?

This makes use of tox in CI systems easy since you don't need to convert
e.g. '3.8' to '38'. We also take the opportunity to comment the regex
and add tests.

Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Contributor Author

stephenfin commented Jan 11, 2023

Added support for period in Python factors, along with interpreter implementation prefixes for all of the interpreters listed in INTERPRETER_SHORT_NAMES in packaging.tags (plus rustpython, which isn't yet listed there). Let me know what you think. Happy to follow this up with a patch to reintroduce documentation explaining factors and what's allowed.

@stephenfin stephenfin marked this pull request as ready for review January 11, 2023 12:26
@gaborbernat
Copy link
Member

Would this be a reasonable compromise: support py, pypy and (purely for legacy reasons) jython but insist on base_python for anything else? We can always expand this regex if a particular interpreter gains enough of a following to warrant first-class support in the future.

Perhaps we can use for start the same thing packaging defines here ttps://github.com/pypa/packaging/blob/63e657138d30063bd46ed16a2e10cfe279a1d7c3/src/packaging/tags.py#L31-L37?

Oh, wait, you're arguing for tox -e 3.8 instead of tox -e py3.8

Ideally but I can live with py3.8 as a compromise.

It's also unnecessary. For one, we could easily do this via a plugin like the old tox-travis plugin. A big part of the tox 4 rewrite (as I understand it) was to improve the plugin architecture: surely the use of plugins should be encouraged?

For this simple use, I'd instead not require users to use a plugin.

You don't need the plugin though. All I've been doing in my CI jobs is running tox -e py. The py environment is special since it implies base_python is whatever sys.executable points to which, as we all know, is determined by the version tox is running (and was installed) under. Install tox under the correct Python version and it'll happily do its job:

This works as long as tox support your targeted Python, but what if you want to run your test suite against say 3.6? I'd rather use a different paradigm, that is more flexible, use the latest python to run tox, and make only the test env run in the target Python - this is what this project does too, check out https://github.com/tox-dev/tox/blob/main/.github/workflows/check.yml#L30-L43; bonus point for tox can take advantage of speed improvements in newer Pythons even when testing previous versions.

Is this somehow insufficient? I mean, couldn't we document this as our recommended practice?

We can do that, but I'd add my approach for reasons outlined in my previous paragraph.

Another point on this: this only works if you're using simple, single-factor testenvs. It doesn't work when you start taking advantage of combinatorial factors, which is an first class feature of tox.

Yeah, but those cases are still, in my experience 20%. They can have more complicated CI configs, but for the 80%, I prefer my approach.

The ymyzk/tox-gh-actions project already exists and supports tox 4 (I'm using it in a few projects of my own, funnily enough). It's far more powerful than simple support for e.g. 3.8 factors. Perhaps we could reach out to the author and ask if they'd like to move it to the @tox-dev organisation, making it a "blessed" plugin (similar to pypa/setuptools_scm in the @pypa organisation)? We could also just link to it from the docs?

I don't like how that project achieves this, and I don't believe we'd need a plugin for this. So I'm soft against it. I reached out to the person over a year ago and they refused, so alas 🦐 , hence why we have the competing https://github.com/tox-dev/tox-gh. But again I'd prefer to not need a plugin for the 80% case.

gaborbernat and others added 2 commits January 11, 2023 07:38
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
@stephenfin
Copy link
Contributor Author

🥳

@stephenfin stephenfin deleted the simple-factors branch January 11, 2023 16:12
descope bot added a commit to descope/django-descope that referenced this pull request Jan 28, 2023
This PR contains the following updates:

| Package | Type | Update | Change | Pending |
|---|---|---|---|---|
| [tox](https://togithub.com/tox-dev/tox)
([changelog](https://tox.wiki/en/latest/changelog.html)) | dev | patch |
`4.2.6` -> `4.2.8` | `4.4.2` (+8) |

---

### Release Notes

<details>
<summary>tox-dev/tox</summary>

### [`v4.2.8`](https://togithub.com/tox-dev/tox/releases/tag/4.2.8)

[Compare Source](https://togithub.com/tox-dev/tox/compare/4.2.7...4.2.8)

#### What's Changed

- Allow package names with env markers with pip binary options by
[@&#8203;q0w](https://togithub.com/q0w) in
[tox-dev/tox#2853

**Full Changelog**: tox-dev/tox@4.2.7...4.2.8

### [`v4.2.7`](https://togithub.com/tox-dev/tox/releases/tag/4.2.7)

[Compare Source](https://togithub.com/tox-dev/tox/compare/4.2.6...4.2.7)

#### What's Changed

- Add release notes project URL for quick access in PyPI web by
[@&#8203;scop](https://togithub.com/scop) in
[tox-dev/tox#2835
- Document colorization regression on Windows by
[@&#8203;adamchainz](https://togithub.com/adamchainz) in
[tox-dev/tox#2837
- The tests actually require wheel by
[@&#8203;hroncok](https://togithub.com/hroncok) in
[tox-dev/tox#2843
- Only return Python factor on base_python conflict by
[@&#8203;stephenfin](https://togithub.com/stephenfin) in
[tox-dev/tox#2840
- Revert to supporting simple Python factors by
[@&#8203;stephenfin](https://togithub.com/stephenfin) in
[tox-dev/tox#2849

**Full Changelog**: tox-dev/tox@4.2.6...4.2.7

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, 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

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

Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants