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

fix: rebuild env if making an incompatible change #781

Merged
merged 7 commits into from
Feb 28, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Feb 23, 2024

This is a followup to #762. Switching environments is likely to be a bit more common, so let's handle it correctly. This has a key path comparison fix that I think was stopping this from being the default before, and instead available via a secret environment variable used in tests.

This is also now "smart" about switching between compatible backends. Switching from venv to virtualenv or back won't trigger a rebuild.

@slafs, could you check this?

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@slafs
Copy link

slafs commented Feb 23, 2024

@henryiii sorry, won't be able to, until at least Tuesday. I'm on vacay without a laptop.

@henryiii
Copy link
Collaborator Author

Okay, enjoy! I think there are enough tests to be pretty safe if everything passes.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii marked this pull request as ready for review February 23, 2024 22:22
@cjolowicz
Copy link
Collaborator

cjolowicz commented Feb 24, 2024

Didn't that environment variable also control whether we check the interpreter version before reuse? IIRC that logic was subtly broken, which is why we put it behind the environment variable in the first place, until someone could come fix it.

For context: #449 (comment)

If this fixes the issue for good, 🎉 :)

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 24, 2024

Yes, that’s the os.path.samefile fix above. The two ways of computing the interpreter path don’t textually compare, but they are logically the same path. And it’s tested (that it is not recreated if unchanged) now too.

@cjolowicz
Copy link
Collaborator

Yes, that’s the os.path.samefile fix above.

I'm not sure I understand how that addresses the issues mentioned in the comment linked above? Specifically, these two:

These issues result in environments never being reused because Nox and virtualenv disagree on which interpreter should be selected. Nox does a PATH lookup, while virtualenv favors the interpreter it was installed on, and sometimes uses stale entries from its cache because it doesn't understand pyenv's shims.

@henryiii
Copy link
Collaborator Author

These issues result in environments never being reused because Nox and virtualenv disagree on which interpreter should be selected

I’ll check, but what I was seeing was that the simple comparison was always failing due to one being resolved differently than the other, but they were the same file. So it was always recreating for me, and the fix above fixed it. I have pyenv but not using it here, so might have missed that case.

We do require virtualenv 20+ now IIRC, so we could follow the suggestion there and use its discovery system, I think?

@henryiii
Copy link
Collaborator Author

(I can also just keep the interp check behind a flag)

@cjolowicz
Copy link
Collaborator

(I can also just keep the interp check behind a flag)

Yeah, this way we can land the environment type check right away and profit :)

I'm all for tackling the interpreter check as well, but I feel it needs a bit of thought.

@henryiii henryiii marked this pull request as draft February 25, 2024 19:09
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Collaborator Author

Unrelated, but we have a logging issue in the tests. On a test failure (possibly only in the correct spot) we get tons of logging file closed errors.

@henryiii henryiii force-pushed the henryiii/fix/changeenv branch 2 times, most recently from 4691bdd to e9794f2 Compare February 26, 2024 17:34
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii marked this pull request as ready for review February 28, 2024 00:02
Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Besides the has_conda (see comment), this LGTM!

tests/test_virtualenv.py Outdated Show resolved Hide resolved
Co-authored-by: Claudio Jolowicz <cjolowicz@gmail.com>
@henryiii
Copy link
Collaborator Author

@cjolowicz added your suggestion, feel free to change the "changes requested". :)

@henryiii
Copy link
Collaborator Author

Didn't realize I could do it - okay, going from the LGTM, going to merge and work on the final followup!

@henryiii henryiii merged commit d59e1ac into wntrblm:main Feb 28, 2024
22 checks passed
@henryiii henryiii deleted the henryiii/fix/changeenv branch February 28, 2024 20:42
github-actions bot pushed a commit to msclock/sphinx-deployment that referenced this pull request Mar 4, 2024
## [0.0.20](v0.0.19...v0.0.20) (2024-03-04)

### Chores

* **deps:** bump wntrblm/nox from 2023.04.22 to 2024.03.02 ([#57](#57)) ([d177375](d177375)), closes [wntrblm/nox#762](wntrblm/nox#762) [wntrblm/nox#787](wntrblm/nox#787) [wntrblm/nox#730](wntrblm/nox#730) [wntrblm/nox#780](wntrblm/nox#780) [wntrblm/nox#770](wntrblm/nox#770) [wntrblm/nox#707](wntrblm/nox#707) [wntrblm/nox#687](wntrblm/nox#687) [wntrblm/nox#756](wntrblm/nox#756) [wntrblm/nox#652](wntrblm/nox#652) [wntrblm/nox#712](wntrblm/nox#712) [wntrblm/nox#781](wntrblm/nox#781) [wntrblm/nox#786](wntrblm/nox#786) [wntrblm/nox#684](wntrblm/nox#684) [wntrblm/nox#723](wntrblm/nox#723) [wntrblm/nox#725](wntrblm/nox#725) [wntrblm/nox#714](wntrblm/nox#714) [wntrblm/nox#715](wntrblm/nox#715) [wntrblm/nox#696](wntrblm/nox#696) [wntrblm/nox#774](wntrblm/nox#774) [wntrblm/nox#782](wntrblm/nox#782) [wntrblm/nox#722](wntrblm/nox#722) [wntrblm/nox#724](wntrblm/nox#724) [wntrblm/nox#721](wntrblm/nox#721) [wntrblm/nox#744](wntrblm/nox#744) [wntrblm/nox#738](wntrblm/nox#738) [wntrblm/nox#762](wntrblm/nox#762) [wntrblm/nox#787](wntrblm/nox#787) [wntrblm/nox#730](wntrblm/nox#730) [wntrblm/nox#780](wntrblm/nox#780) [wntrblm/nox#770](wntrblm/nox#770) [wntrblm/nox#707](wntrblm/nox#707) [wntrblm/nox#687](wntrblm/nox#687) [wntrblm/nox#756](wntrblm/nox#756) [wntrblm/nox#652](wntrblm/nox#652) [wntrblm/nox#712](wntrblm/nox#712) [wntrblm/nox#781](wntrblm/nox#781) [wntrblm/nox#786](wntrblm/nox#786) [wntrblm/nox#684](wntrblm/nox#684) [wntrblm/nox#723](wntrblm/nox#723) [wntrblm/nox#725](wntrblm/nox#725) [wntrblm/nox#714](wntrblm/nox#714) [wntrblm/nox#715](wntrblm/nox#715) [wntrblm/nox#696](wntrblm/nox#696) [wntrblm/nox#774](wntrblm/nox#774) [wntrblm/nox#782](wntrblm/nox#782) [wntrblm/nox#722](wntrblm/nox#722) [wntrblm/nox#724](wntrblm/nox#724) [wntrblm/nox#721](wntrblm/nox#721) [wntrblm/nox#744](wntrblm/nox#744) [#789](https://github.com/msclock/sphinx-deployment/issues/789) [#787](https://github.com/msclock/sphinx-deployment/issues/787) [#786](https://github.com/msclock/sphinx-deployment/issues/786) [#781](https://github.com/msclock/sphinx-deployment/issues/781) [#784](https://github.com/msclock/sphinx-deployment/issues/784) [#780](https://github.com/msclock/sphinx-deployment/issues/780) [#730](https://github.com/msclock/sphinx-deployment/issues/730) [#782](https://github.com/msclock/sphinx-deployment/issues/782) [#783](https://github.com/msclock/sphinx-deployment/issues/783) [#762](https://github.com/msclock/sphinx-deployment/issues/762)

### CI

* set proper permission for preview job ([#56](#56)) ([d65e8a1](d65e8a1))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants