Tox v4.14.1 is no longer expanding {envtmpdir} (and potentially other variables) #3238
Comments
|
No need to ping |
|
Sorry if the ping was seen as aggressive, that was not the intent! I assumed you would be well placed to understand what's going on here, and interested in knowing about this. Regarding the PR, I have no idea how tox works internally so I'll have to leave that to another contributor! Thank you for your work on tox, it is working very well for us 😃 (I just realized you are the main author of the whole package, and not just the PR mentioned above) |
|
I maintain this project, I already get notified of new issues. That being said, I maintain the project, that doesn't mean I do all the development. We don't have today any active developers, so realistically, unless you put in a PR, this might take a while... this is an open-source project kept alive by its community, we have no paid staff to troubleshoot and resolve new issues. |
gaborbernat
added
bug:minor
|
Furthermore, creating a repository that I can clone to reproduce would significantly increase the chances of getting traction. Thanks! |
|
Here is a minimal reproduction repository: https://github.com/Luthaf/tox-variables I had to add a separate |
|
Thanks for that 🙏 |
|
I have done a bit of further digging based off @felixscherz very helpful information. As he points out, with the change to Lines 163 to 169 in 08223fc the tox/src/tox/session/env_select.py Line 363 in f5850c0 When the packaging environment is actually run, the config is accessed with package=False (the default value) and this causes it to create a new Config object, rather than reusing the original one, which means that everything that happens in tox/src/tox/session/env_select.py Line 367 in f5850c0 doesn't get included in the Config made available during the commands on the packaging environment. @gaborbernat why was there a need to include the |
|
Package and run environments have different base environment. Base environments are important when determining the fallback env. For run env this is |
|
Yes, but the issue here is that a packaging environment like |
|
You're correct. Not entirely sure how to encode that in the interface, suggestions? |
Is this the bug, should we pass in package=True instead? Can we do that? |
|
I think there are a few options available here (with a choice on size of change vs overall quality of fix) - passing tox/src/tox/config/loader/ini/replace.py Lines 277 to 295 in f5850c0 It also seems unnecessary to have to try and retrieve whether package is true over and over when it shouldn't change.
I think the simplest fix is that suggested by @felixscherz - just don't use Probably a better fix would be to store and retrieve the config directly from the environment that is being run, rather than looking it up in the global cache. I think the env already has access to this conf, but I don't know enough detail on the data flow to figure out where in the chain of calls that leads to calling |
I believe this would be the actual solution, and we need to check how we can pass this information down from the callers side rather than using that default. 🤔
I do not like this approach in general, ignoring argument is not desirable.
Some configuration can be expensive to calculate. That is why we have the cash so that we don't do redundant work unless we must. |
The point is that the argument is only relevant when creating the config for an environment, and at no point afterwards. At the moment the actual env config is created here: tox/src/tox/session/env_select.py Line 363 in f5850c0 which is paired with the matching line here for creating a run env config: tox/src/tox/session/env_select.py Line 316 in f5850c0 Could we split the current get_env() function into a create_env() and get_env() pair, where create_env() is called in the two places above, and accepts a package argument (or even just a base argument directly) and everywhere else uses get_env() which drops package altogether and raises a KeyError if asked for an env which does not yet exist, rather than creating it on demand? As far as I can tell that would better describe the current logic as there don't appear to be any cases where calling the current get_env() should sometimes create and sometimes retrieve the result, it is always the case that it is first called from tox/session/env_select.py first to create it, and then from config/loader/ini/replace.py to retrieve it.
But the |
I should also point out that in the relevant function: Lines 126 to 146 in 08223fc The loaders argument is already being ignored when retrieving from the cache, it is only used to build it the first time, and the base argument was previously being ignored except for the first time, so it is not like this is without precedent ;-)
|
This is not true. If you look deeper into the code you will realize that we are actually doing a two-phase environment creation. If we first discover an environment we assume it's going to be running environment and then if at the later point running environment marks that as a packaging we will convert it to a packaging environment. |
That could work. |
I may have missed something in the depths of the code, but on all my tests the
Going through the code to make changes, this is unfortunately not enough by itself - the current
I think option 2 is the simplest and cleanest and I will put together a PR to implement that, unless anyone has a counter-proposal? It would be very helpful to get some advice on how to run a suitable test and where to put it within the test folder structure. It could go into |
|
I'd be ok to see a 2 proposals implementation, but also 1. I'm against 3. |
gaborbernat
added
the
level:hard
|
xref possible duplicate: #3292 |
|
@bennyrowland Do you still plan to work on this? |
|
So, having identified this as a duplicate, here's a comment I made over in the other issue. I believe, that my reproducer is simpler:
Originally posted in #3292 (comment) Hey @hynek, so Bernát confirmed that |
|
Another observation with the repro above: $ tox c -e bug-demo
ROOT: HandledError| .pkg cannot self-package(while |
Tox v4.14.1 regression ref: tox-dev/tox#3238
So just “Replacing |
|
For 99%, yes. |
Tox v4.14.1 regression ref: tox-dev/tox#3238
Tox v4.14.1 regression ref: tox-dev/tox#3238
Issue
We are using
package = externalandpackage_env = build-metatensor-corein our tox setup, and build the wheels withpip wheel python/metatensor-core {[testenv]build_single_wheel_flags} --wheel-dir {envtmpdir}/distOn tox 4.14.0, everything is fine, on 4.14.1 tox creates a directory literally named
{envtmpdir}/dist(instead of expanding this to.tox/build-metatensor-core/tmp/dist.Environment
Provide at least:
Output of
pip listof the host Python, wheretoxis installedOutput of running tox
Output of
tox -rvvPing @gaborbernat, this seems to be a fallout of #3237
The text was updated successfully, but these errors were encountered: