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 should not make major backwards incompatible / undocumented / undeprecated changes on minor patch releases (passenv change) #2676

Closed
zzzeek opened this issue Dec 11, 2022 · 11 comments

Comments

@zzzeek
Copy link

zzzeek commented Dec 11, 2022

upgrading from tox 4.0.5 to tox 4.0.6 produces errors for what appears to be an unannounced change which is that the passenv variable does not allow the names to be space-separated.

looking at the commit in f4840a0, there was no prior deprecation warning and no opportunity for projects to accommodate for this major behavioral change. SQLAlchemy has had a space-separated list of names here for many years and we had no idea at all that this was not correct. I'm not readily finding documentation for this, searching the page https://tox.wiki/en/latest/config.html for the terms "comma" or "space" shows nothing, which indicates there is no guidance on how values should be separated in such variables.

I would ask that major backwards incompatible changes like these please go through a normal deprecation phase and additionally use warnings, for at least a full major version, to provide a timely note for projects to update their tox configurations.

Personally I would revert this change in tox and wait for at least 4.1, however that's up to tox maintainers, we will have to likely release a post release of SQLAlchemy 1.4.45 to work around this regardless.

@zzzeek zzzeek changed the title tox should not make major backwards incompatible / undocumented / undeprecated changes on minor point releases (passenv change) tox should not make major backwards incompatible / undocumented / undeprecated changes on minor patch releases (passenv change) Dec 11, 2022
@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

it seems to be here:

https://tox.wiki/en/latest/faq.html#tox-4-changed-ini-rules

however only took effect as of 4.0.6. a deprecation warning would have been very helpful

@jugmac00
Copy link
Member

Hey @zzzeek

I am sorry you encounter issues with the new version.

Quick answer as I am on my smartphone.

The breaking change was between V3 and V4, and is documented at least in the breaking changes section
https://tox.wiki/en/latest/faq.html#tox-4-changed-ini-rules

Breaking change is a strong word anyway, as afaik space separation probably worked in V3 by chance, and was not even documented. (Cannot look that up on mobile, but I think I remember it that way). As v4 is a rewrite, we were not aware of all undocumented features. That is also a reason we asked many times for testing pre-releases, with more or less success.

It was broken on v4, but - and that is what your referenced commit did, raise a proper error message. Before that it just did not work properly and additionally did not raise an error.

@gaborbernat
Copy link
Member

The breaking change happened with 4.0. the latest minor just made the silent error not silent.

@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

Best practice here is when such a change is identified, especially for a project as widely used as tox, to assume people are using it and relying upon it. You put in a deprecation warning that will clearly emit during the 3.x series. That way we would all have known about it. pytest is pretty clear on warning for things like this, I would follow their general methodology.

@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

I identify unexpected behaviors in SQLAlchemy all the time. We always emit deprecation warnings for a whole major cycle (a year) before changing them.

@gaborbernat
Copy link
Member

gaborbernat commented Dec 11, 2022

Best practice here is when such a change is identified, especially for a project as widely used as tox, to assume people are using it and relying upon it. You put in a deprecation warning that will clearly emit during the 3.x series.

But this is not deprecated in version 3. It's only not supported any more in version 4. You can make breaking changes in new significant releases. Before 4.0.6 we silently ignored the failure, but some people requested in #2658 to be a hard failure instead.

I identify unexpected behaviors in SQLAlchemy all the time. We always emit deprecation warnings for a whole major cycle (a year) before changing them.

You're free to do as you wish your project you maintain. However, please respect our methodologies too. Remember, this is a free, open-source project that's unpaid. Either change your configuration to be tox 4 compliant or pin to version 3.

@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

Best practice here is when such a change is identified, especially for a project as widely used as tox, to assume people are using it and relying upon it. You put in a deprecation warning that will clearly emit during the 3.x series.

But this is not deprecated in version 3. It's only not supported any more in version 4. You can make breaking changes in new significant releases. Before 4.0.6 we silently ignored the failure, but some people requested in #2658 to be a hard failure instead.

yes I understand that now, so we were broken for all 4.0 releases, that is fine. however, the phrase "this is not deprecated in version 3. It's only not supported any more in version 4" does not make any sense.

You can make breaking changes in new significant releases.

absolutely, although it is very good practice to warn about such changes in the previous major release, again see pytest for how they approach major changes like this. If tox is going to be making more surprise changes like this that we are not being alerted towards, this decreases the viability of tox as a major foundational component.

as it stands, the first place I looked for this change was the actual passenv documentation: https://tox.wiki/en/latest/config.html#pass_env no mention of the change is made here either. Sphinx documentation supports the ` ..deprecated`` directive that can be used for this, which you can see for example in Python's documentation at https://docs.python.org/3/library/importlib.html#importlib.abc.Finder .

You're free to do as you wish your project you maintain. However, please respect our methodologies too. Remember, this is a free, open-source project that's unpaid. Either change your configuration to be tox 4 compliant or pin to version 3.

I'm only pointing out that your methodology in this instance is falling short of best practices given the foundational nature of tox as an open source project.

@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

do commas as separator work in tox 3 ? I am getting missing environment variables under tox 3 when using commas in passenv

@zzzeek
Copy link
Author

zzzeek commented Dec 11, 2022

I see, it's documented explicitly as space separated at https://tox.wiki/en/3.1.0/config.html#confval-passenv=SPACE-SEPARATED-GLOBNAMES

wow

so it's impossible to have a tox.ini file that works under tox 3 and tox 4 at the same time ?

@gaborbernat
Copy link
Member

so it's impossible to have a tox.ini file that works under tox 3 and tox 4 at the same time ?

Not true. That's a bug in our documentation. Also note the more up to date link https://tox.wiki/en/legacy/config.html#conf-passenv

You can use line separation both in tox 3 and 4. https://github.com/tox-dev/tox/blob/main/tox.ini#L19-L21

@tox-dev tox-dev locked as too heated and limited conversation to collaborators Dec 11, 2022
@gaborbernat
Copy link
Member

gaborbernat commented Dec 11, 2022

@zzzeek I'm locking the conversation here because I'm not a big fan of your accusatory conversation style, rather than trying to work with us and help out to make things better. You've been presented with paths ahead; take one that suits you.

so it's impossible to have a tox.ini file that works under tox 3 and tox 4 at the same time ?

There's no true expectation here to have a tox.ini that works both with v3 and v4. The ideal migration path is that you make your configuration file tox 4 compliant and then set minversion=4 and let tox 3 die.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants