Ignore basepython for default factors (#477) by stephenfin · Pull Request #841 · tox-dev/tox

stephenfin · 2018-06-08T19:57:08Z

tox provides a number of default factors - py27, py34, py35 etc. - that
are tied to particular interpreter versions. It is possible to override
these through individual sections or the global [testenv] section. For
example, consider the following 'tox.ini' file:

[tox]
skipsdist = True
minversion = 2.0
distribute = False
envlist = py35,py27,pep8,py34-test

[testenv]
basepython = python3
install_command = pip install {opts} {packages}
commands =
python --version

[testenv:py27]
basepython = python2.7

Running any target except for 'py27' will result in the same interpreter
being used. On Fedora 28 with the 'python3-tox' package:

$ tox -qq -e py27
Python 2.7.15
$ tox -qq -e py35
Python 3.6.5
$ tox -qq -e py34-test
Python 3.6.5

This is broken by design. Overriding these makes no sense and is a
source of common misconfigurations, as noted in #477. The only sane
thing to do here is ignore the request and use the correct interpreter
so this is what's done.

codecov · 2018-06-08T20:12:06Z

Codecov Report

Merging #841 into master will increase coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #841    +/-   ##
======================================
+ Coverage      92%    92%   +<1%     
======================================
  Files          13     13            
  Lines        2415   2420     +5     
  Branches      428    430     +2     
======================================
+ Hits         2215   2220     +5     
  Misses        124    124            
  Partials       76     76

gaborbernat · 2018-06-09T11:40:25Z

@stephenfin thank you very much for the configuration. I would prefer to raise an error for invalid configuration though; instead of ignoring it. @obestwalter agree?

stephenfin · 2018-06-09T12:52:44Z

@stephenfin thank you very much for the configuration. I would prefer to raise an error for invalid configuration though; instead of ignoring it. @obestwalter agree?

I considered that but decided not to because I wanted to allow configurations like the following:

[tox]
envlist = py27,py35,py36,docs,style,coverage

[testenv]
basepython = python3
commands = pytest ...

[testenv:docs]
commands = sphinx-build ...

[testenv:coverage]
commands = coverage ...

[testenv:style]
commands = flake8 ...

e.g. I would be able to ensure everything except the Python-versioned environments would use Python 3, to assist in migrations to Python 3. This has come up repeatedly in OpenStack, as seen here and here, and simply overriding things allows us to keep our tox.ini smaller.

That said, I can see how this could be confusing. I would have added a DEBUG-level log, but it seems you're not using any logging infrastructure so I can't. I'll leave the decision on this to you.

gaborbernat

@stephenfin I see your point; I say let's follow the zen of Python: explicit better than implicit. So instead of blindly ignoring things let's have a way to opt-in into such decisions.

basepython_env_name_missmatch = raise | ignore | override

Where raise aborts with a config error; ignore has the current strategy, and override can be what you're proposing here. Let's leave ignore as default for now (we can change it when we do a major release), and we should also expose this option as a command line argument to facilitate with testing/debugging. Does that seem reasonable to you?

stephenfin · 2018-06-18T20:16:48Z

@stephenfin I see your point; I say let's follow the zen of Python: explicit better than implicit. So instead of blindly ignoring things let's have a way to opt-in into such decisions.
basepython_env_name_missmatch = raise | ignore | override
Where raise aborts with a config error; ignore has the current strategy, and override can be what you're proposing here. Let's leave ignore as default for now (we can change it when we do a major release), and we should also expose this option as a command line argument to facilitate with testing/debugging. Does that seem reasonable to you?

That sounds mostly OK but I'd like to propose a slight tweak.

ignore_basepython_conflict = True | False (default)

Where False preserves the current behaviour in this version but raises a warning and True gives me the behaviour I described above. Once tox 4.0 is released, we could bump this warning to an exception. What do you think?

EDIT The reason I'd prefer this approach is because I think overridding the default env is never the correct thing to do. We should either ignore these overrides or error out. This approach provides a path there.

gaborbernat · 2018-06-19T06:30:05Z

Ok. Make the changes and we can roll with it.

stephenfin · 2018-06-19T08:24:50Z

Ok. Make the changes and we can roll with it.

These are done now (I force pushed). The CI is failing for what looks like unrelated reasons. Any idea what's going on?

gaborbernat · 2018-06-19T08:45:46Z

doc/conf.py

    # from sphinx.ext.autodoc import cut_lines
    # app.connect('autodoc-process-docstring', cut_lines(4, what=['module']))
-    app.add_description_unit(
+    app.add_object_type(


please explain this change 👍

See 116afb1

gaborbernat · 2018-06-19T08:45:52Z

doc/config.rst

 .. confval:: commands=ARGVLIST

-    the commands to be called for testing. Each command is defined
+    The commands to be called for testing. Each command is defined


please explain this change 👍

See f52eea7 (it's just me scratching my OCD itch before I modify the document)

gaborbernat · 2018-06-19T08:45:58Z

doc/config.rst

-      further commands will be executed (which was the default behavior in tox <
-      2.0).  If ``False`` (the default), then a non-zero exit code from one command
-      will abort execution of commands for that environment.
+    If ``True``, a non-zero exit code from one command will be ignored and


please explain this change 👍

gaborbernat · 2018-06-19T08:46:03Z

doc/config.rst

-   variable doesn't exist in the tox invocation environment it is ignored.
-   You can use ``*`` and ``?`` to match multiple environment variables with
-   one name.
+    A list of wildcard environment variable names which


please explain this change 👍

gaborbernat · 2018-06-19T08:46:10Z

doc/config.rst

 .. confval:: downloadcache=path

-    **IGNORED** -- Since pip-8 has caching by default this option is now ignored.  Please remove it from your configs as a future tox version might bark on it.
+    **IGNORED** -- Since pip-8 has caching by default this option is now


please explain this change 👍

gaborbernat · 2018-06-19T08:47:09Z

src/tox/config.py

+
+                if str(value) != default:
+                    # TODO(stephenfin): Raise an exception here in tox 4.0
+                    print(


let's use the warning system instead, https://docs.python.org/2/library/warnings.html; as the user can turn off that if they want

Sounds good. I wasn't able to find any other examples of warnings in use so I assumed you didn't use them.

gaborbernat · 2018-06-19T08:47:51Z

tox.ini

 whitelist_externals = sphinx-build
 basepython = python3.6
 extras = docs
-changedir = {toxinidir}


why did you remove this, I think that's why things fail now

Signed-off-by: Stephen Finucane <stephen@that.guru>

tox provides a number of default factors - py27, py34, py35 etc. - that are tied to particular interpreter versions. It is possible to override these through individual sections or the global [testenv] section. For example, consider the following 'tox.ini' file: [tox] skipsdist = True minversion = 2.0 distribute = False envlist = py35,py27,pep8,py34-test [testenv] basepython = python3 install_command = pip install {opts} {packages} commands = python --version [testenv:py27] basepython = python2.7 Running any target except for 'py27' will result in the same interpreter being used. On Fedora 28 with the 'python3-tox' package: $ tox -qq -e py27 Python 2.7.15 $ tox -qq -e py35 Python 3.6.5 $ tox -qq -e py34-test Python 3.6.5 This is broken by design. Overriding these makes no sense and is a source of common misconfigurations, as noted in tox-dev#477. The only sane thing to do here is ignore the request and use the correct interpreter or raise a warning. There is merit to both approaches, so this functionality is exposed by way of a new global configuration option, 'ignore_basepython_conflict'. Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#477

The 'add_description_unit' is deprecated for removal in Sphinx 2.0. Get ahead of the curve and use its replacement now. Signed-off-by: Stephen Finucane <stephen@that.guru>

This lets us pass things like '-E' (to discard environment) when needed. Signed-off-by: Stephen Finucane <stephen@that.guru>

This is a common name for a virtual environment used for testing. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin · 2018-06-19T09:49:42Z

@gaborbernat I have a bad habit of using GitHub like I use mailing lists and Gerrit (including force pushes) 😄 The answer to all your questions above is in the commit messages. Is this what you're looking for?

gaborbernat · 2018-06-19T09:53:21Z

Yeah, migrate to the warnings api and we're good.

This is the preferred way to do this. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin · 2018-06-19T15:14:40Z

Yeah, migrate to the warnings api and we're good.

This is done now. I even kept it separate, even though it hurt to do so. Stupid GitHub Pull Request workflow 😅

gaborbernat · 2018-06-19T16:12:09Z

@stephenfin thank you very much!

…thon.exe https://nedbatchelder.com/blog/201509/appveyor.html The old TOXPYTHON trick seems to be discouraged now with tox 3.1, which emits a warning when there is a conflict in the basepython settings for environments containing default factors (e.g. py27, etc.) tox-dev/tox#841

The following is intended to be a correct tox configuration: [tox] min_version = 4.1 env_list = py{38,39,310,311},docs [testenv] base_python = python3 [testenv:docs] commands = sphinx-build ... The goal of this is to use 'python3' (i.e. the value of '[testenv] base_python') for all environments except those with specific 'pyXX' factors in them. This helps eliminate issues where environments that do not specify a 'pyXX' factor run with different Python versions in different environments. An earlier bug, tox-dev#477 [1], prevented us from doing this. Due to tox-dev#477, the interpreter version indicated by '[testenv] base_python' (or rather '[testenv] basepython' - the underscore-separated variant was only introduced in tox 4) would override the value indicated by the 'pyXX' factor. This was resolved with a PR, tox-dev#841 [2], which started warning users if they were unintentionally running against the wrong interpreter and introduced the 'ignore_basepython_conflict' value to opt into the correct behavior. Unfortunately, this has been broken in the move to tox 4. Currently, running with the above configuration will result in 'python3' being used for every environment. This is clearly incorrect. This issue stems from an errant bit of logic. When comparing interpreter versions as part of the '_validate_base_python', we ignore various attributes if they are unset on either '[testenv] base_python' or the 'pyXX' factor. This allows e.g. 'python3' to match 'python3.8', which isn't correct. Correct this by insisting on matching on all attributes that are set on the factor. [1] tox-dev#477 [2] tox-dev#841 Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#2754

The following is intended to be a correct tox configuration: [tox] min_version = 4.1 env_list = py{38,39,310,311},docs [testenv] base_python = python3.8 [testenv:docs] commands = sphinx-build ... The goal of this is to use 'python3.8' (i.e. the value of '[testenv] base_python') for all environments except those with specific 'pyXX' factors in them. This helps eliminate issues where environments that do not specify a 'pyXX' factor run with different Python versions in different environments. An earlier bug, tox-dev#477 [1], prevented us from doing this. Due to tox-dev#477, the interpreter version indicated by '[testenv] base_python' (or rather '[testenv] basepython' - the underscore-separated variant was only introduced in tox 4) would override the value indicated by the 'pyXX' factor. This was resolved with a PR, tox-dev#841 [2], which started warning users if they were unintentionally running against the wrong interpreter and introduced the 'ignore_basepython_conflict' value to opt into the correct behavior. Unfortunately, this has been partially broken in the move to tox 4. While the above configuration still works, the following no longer does: [tox] min_version = 4.1 env_list = py{38,39,310,311},docs [testenv] base_python = python3 [testenv:docs] commands = sphinx-build ... This configuration was common back during the move from Python 2 to Python 3. It ensured that 'python3' was used for all testenvs that did not request an explicit Python version via a factor or their own 'base_python' version. While it's no longer necessary, it is still quite common. Currently, running with this configuration will result in 'python3' being used for every environment, rather than e.g. 'python3.8' for a testenv with the 'py38' factor. This is clearly incorrect. This issue stems from an errant bit of logic. When comparing interpreter versions as part of the '_validate_base_python', we ignore various attributes if they are unset on either '[testenv] base_python' or the 'pyXX' factor. This allows e.g. 'python3' to match 'python3.8', since the minor version is unset for the '[testenv] base_python' value, 'python3'. The fix is simple: while we can ignore unset attributes for factor-derived versions (to allow a 'py3' factor to work with e.g. 'base_python = python3.8'), we should insist on them for 'base_python'-derived versions. [1] tox-dev#477 [2] tox-dev#841 Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#2754

stephenfin force-pushed the issues/477 branch from 214d0d9 to 440e624 Compare June 8, 2018 20:12

stephenfin force-pushed the issues/477 branch from 440e624 to 882146f Compare June 8, 2018 20:42

gaborbernat requested review from gaborbernat and obestwalter June 9, 2018 11:40

gaborbernat assigned stephenfin Jun 9, 2018

gaborbernat added bug:normal affects many people or has quite an impact feature:change something exists already but should behave differently area:configuration level:medium rought estimate that this might be neither easy nor hard to implement labels Jun 9, 2018

gaborbernat requested changes Jun 10, 2018

View reviewed changes

stephenfin force-pushed the issues/477 branch from 882146f to ec86041 Compare June 18, 2018 22:34

gaborbernat requested changes Jun 19, 2018

View reviewed changes

stephenfin and others added 6 commits June 19, 2018 09:50

trivial: Fix indentation/alignment in doc/config.rst

f52eea7

Signed-off-by: Stephen Finucane <stephen@that.guru>

Replace deprecated API

116afb1

The 'add_description_unit' is deprecated for removal in Sphinx 2.0. Get ahead of the curve and use its replacement now. Signed-off-by: Stephen Finucane <stephen@that.guru>

Pass posargs to 'docs' target

31bd1dd

This lets us pass things like '-E' (to discard environment) when needed. Signed-off-by: Stephen Finucane <stephen@that.guru>

gitignore: Ignore .venv

59cd116

This is a common name for a virtual environment used for testing. Signed-off-by: Stephen Finucane <stephen@that.guru>

revert docs env tox.ini change

59fe2a2

gaborbernat force-pushed the issues/477 branch from ec86041 to 59fe2a2 Compare June 19, 2018 08:52

Use warnings for deprecated features

7280a86

This is the preferred way to do this. Signed-off-by: Stephen Finucane <stephen@that.guru>

gaborbernat approved these changes Jun 19, 2018

View reviewed changes

gaborbernat merged commit d5b9c0a into tox-dev:master Jun 19, 2018

stephenfin deleted the issues/477 branch June 20, 2018 09:02

stephenfin mentioned this pull request Jan 5, 2023

Fail on mismatched python spec attributes #2824

Merged

This was referenced Jan 10, 2023

Specify default Python(s) #2846

Closed

Only return Python factor on base_python conflict #2840

Merged

Uh oh!

Conversation

stephenfin commented Jun 8, 2018

Uh oh!

codecov bot commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gaborbernat commented Jun 9, 2018

Uh oh!

stephenfin commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborbernat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborbernat commented Jun 19, 2018

Uh oh!

stephenfin commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin commented Jun 19, 2018

Uh oh!

gaborbernat commented Jun 19, 2018

Uh oh!

stephenfin commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborbernat commented Jun 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jun 8, 2018 •

edited

Loading

stephenfin commented Jun 9, 2018 •

edited

Loading

gaborbernat left a comment •

edited

Loading

stephenfin commented Jun 18, 2018 •

edited

Loading

stephenfin Jun 19, 2018 •

edited

Loading

stephenfin commented Jun 19, 2018 •

edited

Loading