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

Tox 3.1 regression #906

Closed
benjaoming opened this issue Jul 12, 2018 · 10 comments
Closed

Tox 3.1 regression #906

benjaoming opened this issue Jul 12, 2018 · 10 comments
Labels
bug:normal affects many people or has quite an impact

Comments

@benjaoming
Copy link

benjaoming commented Jul 12, 2018

Observation

In django-money/django-money#433, I downgraded Tox because previous behavior is failing.

It seems since 3.1 that all our [django] sections in tox.ini are now concatenated, rather than picked individually.

So this is the actual result:

django111-py34 installdeps: .[test], django-reversion==1.10.0, djangorestframework>=3.3.3,<3.7.0, Django>=1.11.0,<2.0.0, django-reversion>=2.0.8, djangorestframework>=3.6.2, django-reversion>=2.0.8, djangorestframework>=3.7.3, django-reversion>=2.0.8, djangorestframework>=3.6.2

And this is the expected result:

django111-py34 installdeps: .[test], Django>=1.11.0,<2.0.0, django-reversion>=2.0.8, djangorestframework>=3.6.2

Minimal reproducible example or detailed description, assign "bug"

[tox]
envlist =
    django_master-py{36,35}
    django20-py{36,35,34,py3}
    django111-py{36,35,34,27,py}
    django18-py{35,34,27,py}
    lint
    docs
skipsdist = true

[testenv]
deps =
    .[test]
    django18: {[django]1.8.x}
    django111: {[django]1.11.x}
    django20: {[django]2.0.x}
    django_master: {[django]master}
commands = py.test --ds=tests.settings --cov=./djmoney {posargs}
usedevelop = false

[django]
1.8.x  =
       Django>=1.8.0,<1.9.0
       django-reversion==1.10.0
       djangorestframework>=3.3.3,<3.7.0
1.11.x  =
       Django>=1.11.0,<2.0.0
       django-reversion>=2.0.8
       djangorestframework>=3.6.2
2.0.x  =
       Django>=2.0,<2.1
       django-reversion>=2.0.8
       djangorestframework>=3.7.3
master =
       https://github.com/django/django/tarball/master
       django-reversion>=2.0.8
       djangorestframework>=3.6.2

OS and pip list output

@obestwalter obestwalter added the bug:normal affects many people or has quite an impact label Jul 12, 2018
@obestwalter
Copy link
Member

@benjaoming - thanks for the report. I have to admit that this is the first time I see this django section, so this looks like an undocumented feature of tox to me (not the first one for sure).

We'll release a fix later today that reverts part of the substitution behaviour, which could possibly also fix this problem. We'll see. If not, we'll look into this after we fixed #899

@gaborbernat
Copy link
Member

It's not the first time either we run into issues with the config file sytnax. Maybe we should freeze the config file format and start with a fresh start via pyproject.toml.

@obestwalter
Copy link
Member

Sounds famliar :)

#600

@benjaoming
Copy link
Author

I didn't author our tox.ini from reading docs... I don't even remember writing it, I just co-maintain a project :) Should it be written differently? I'm fine about changing our config to something that is expected to work, rather than something that works unintentionally and breaks again all the same :)

@obestwalter
Copy link
Member

Don't worry about it @benjaoming - all I can say is that I never have seen this usage befrore. We'll have to fix this anyway because we don't want to break configurations in the wild without warning. I will be preparign the relase now and will check it against your project to see if that also fixes your problem. If not there will be a bugfix for it soon.

@benjaoming
Copy link
Author

Thanks a lot @obestwalter ! :)

You could also consider to detect and deprecate unwanted syntax (through warning messages) such that it can be finally cleaned up in say 3.2 or 3.3.

@obestwalter
Copy link
Member

obestwalter commented Jul 12, 2018

Definitely a good way to handle it - unfortunately we didn't even know that this syntax was supported 😆

I think we might actually feature freeze the old tox.ini config format at some point and move to a new format that is better defined and does not use a home grown parser. This would guarantee that the old way of configuring things stays stable for existing users that want to keep the tox.ini and we don't have regressions when we touch the parsing and substituion code.

@benjaoming
Copy link
Author

A bit OT: No shame in writing your own parser when the syntax is simple, easy to read and does the job :) What I really like about tox.ini syntax is that I never read the docs for the syntax, I just used bits and pieces from other people's configs and kind of guessed how it worked. Am glad that you are keeping it, and yes if it ain't broken don't fix it -- could be applied in the sense that it should also be feature frozen to avoid breakage. I never met any limitations to the config syntax in my Django world.

Smiles from a happy Tox user :)

@obestwalter
Copy link
Member

I just cloned django-money and tested with 3.1.2 - I think we can close this as fixed for now.

@blueyed
Copy link

blueyed commented Sep 11, 2018

JFI: Django appears to have been picked up correctly (Django>=1.11.0,<2.0.0), but only the 2nd and 3rd lines where used from all factors.
Was looking into this because of #706.

bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 14, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 14, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 14, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 14, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 20, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 20, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
bartsanchez added a commit to bartsanchez/tox that referenced this issue Oct 20, 2018
New version with improved changes, fixing errors raised
with the previous version:

tox-dev#899
tox-dev#906
gaborbernat pushed a commit that referenced this issue Oct 21, 2018
New version with improved changes, fixing errors raised with the previous version:

#899
#906
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug:normal affects many people or has quite an impact
Projects
None yet
Development

No branches or pull requests

4 participants