-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
2da4843
to
0a6779f
Compare
@henryiii sorry, won't be able to, until at least Tuesday. I'm on vacay without a laptop. |
a895b65
to
b4cdfe3
Compare
Okay, enjoy! I think there are enough tests to be pretty safe if everything passes. |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
b4cdfe3
to
b3a4c37
Compare
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, 🎉 :) |
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. |
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 |
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? |
(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. |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
0b1d22d
to
152bb3d
Compare
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. |
4691bdd
to
e9794f2
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
e9794f2
to
540cdbd
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
db46298
to
7882ee1
Compare
There was a problem hiding this 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!
Co-authored-by: Claudio Jolowicz <cjolowicz@gmail.com>
@cjolowicz added your suggestion, feel free to change the "changes requested". :) |
Didn't realize I could do it - okay, going from the LGTM, going to merge and work on the final followup! |
## [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))
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?