Better (but not completely fixed) treatment of venv #904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
mcdonc
commented
Dec 18, 2023
mcdonc
commented
|
Sorry, I thought I squashed all those commits. I suck at git. |
- get rid of requirements check for recreation - store .devenv_interpreter because the venv python binary resolves to the origin python of the wrapper, not the wrapper python In practice, merging this commit will mean that a) the invocation of pip install -r will need to take care of any changed requirements. b) the venv, once created, will never be recreated unless the Python wrapper changes. The Python wrapper doesn't seem to change when you add Python packages to your packages = list, I'm not sure what the other circumstances might be. In reality, this fix sucks because the venv is not actually based on the wrapper. The wrapper, in fact, seems to never get used. That will need to be addressed in some other commit, though.
|
Nice! If we can get the wrapper to be passed to the virtualenv correctly that will solve all the issues with dynamic linking :) |
|
I wonder, why get rid of requirements checking? I like that it would rebuild the venv if it changes and I'd expect it to. |
The particular form of requirements checking that I took out didn't make too much sense, I don't think. It checked for a literal "requirements.txt" inside the venv root, which can't really work generically. In addition (and maybe unrelatedly), I think I would prefer it if "devenv up" did not try to run pip install -r again. I think that job should be relegated to the flake rebuild. I haven't investigated why this happens yet, though. But the reason I would prefer this is because it just takes way too long on any complicated project to rerun pip install -r. |
If the venv's "python" resolves to the wrapper we won't need the |
|
See also my rationale at #745 (comment) |
|
The reason that the venv's bin/python resolves to the bin/python of the parent of the wrapper (and doesn't resolve to the wrapper's bin/python) is due to how I actually think this might be ok because So delta the question of getting rid of the literal requirements.txt check, I think this PR is ready. EDIT (later)... uh, maybe not. Confusion reigns. I guess the whole point is to execute the wrapper as Python for its side effects. But it really can't be done without creating a custom venv builder (which is definitely possible). I don't think I quite understand what the manylinux stuff is doing under the hood. I'll try to figure it out. |
… to execute the entirety of the cmdline as a single command
|
I think maybe this is another reason to not pip install on devenv up. This is me starting a container that was generated via |
|
It's likely I should close this PR and open several that break out the changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.