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

Cache env config #3268

Closed
wants to merge 3 commits into from
Closed

Conversation

bennyrowland
Copy link

This commit adds a new cache to the main config object, specifically for env config. When env config is retrieved via Config.get_env(), it is not in general possible to determine whether the env is a packaging environment or not. Because retrieving standard section config needs to know which "base" section to inherit from, and this is different for test envs and packaging envs, we require a separate way to retrieve previously created env config. At creation time we do reliably know whether this is a packaging or run env, and we want that to persist (if it changes, we call clear_env() and recreate it with the new type).

At the moment, there are no tests for this change, I would appreciate a) approval of this approach to fixing the problem and b) some guidance about where to put a test (or possibly just add some additional checks to an existing test, to demonstrate e.g. that {envtmpdir} is being expanded). I have verified that my project that got me interested in this issue now builds correctly with this fix.

I don't think any change to the documentation is required for this. I will add a changelog entry along with the tests.

Fixes #3238.

  • 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

This commit adds a new cache to the main config object, specifically for
env config. When env config is retrieved via Config.get_env(), it is
not in general possible to determine whether the env is a packaging
environment or not. Because retrieving standard section config needs to
know which "base" section to inherit from, and this is different for
test envs and packaging envs, we require a separate way to retrieve
previously created env config. At creation time we do reliably know
whether this is a packaging or run env, and we want that to persist (if
it changes, we call clear_env() and recreate it with the new type).
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright, you'd want to create your example in a config and check that tox -c expands correctly.

@bennyrowland
Copy link
Author

Ok, interestingly, tox config does deliver the correct expansion of placeholders - I think because it is not getting as far as running the commands it doesn't re-request the env and so doesn't get the second, incomplete, version.

This means that we will have to make a proper run to test this (AFAICT). I am continuing to work on this (in limited spare time) but it has become more complicated than it first appeared (why are the tests always harder than the fix itself), and may take a bit longer to get to a solution.

@gaborbernat
Copy link
Member

why are the tests always harder than the fix itself

That's expected 😊 writing code is fairly easy compared to maintaining it.

@gaborbernat gaborbernat marked this pull request as draft April 23, 2024 16:44
@gaborbernat
Copy link
Member

Seems stalled, we can reopen when you pick it up.

@webknjaz
Copy link
Contributor

Ok, interestingly, tox config does deliver the correct expansion of placeholders

FTR, I was also observing that tox l expands stuff properly too, not just tox config.

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

Successfully merging this pull request may close these issues.

Tox v4.14.1 is no longer expanding {envtmpdir} (and potentially other variables)
3 participants