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

recreate venv when -rrequire.txt changes #668

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@kapilt

kapilt commented Nov 2, 2017

This seems to be a long standing fundamental issue with tox, thats been long sidelined by calling -r in deps spec experimental, but given its common usage, one worth correcting given its a common pitfall, and reported numerous times per discussion and refs in #149. i took a look at the suggestions in there of various other tools, but they all had various pitfalls, and also took a look at what some other real world projects are doing with tox (like botocore which does a separate installer script in commands to avoid this pitfall) before deciding the simplest thing was just fixing tox, via checksum on requirements files.

@codecov

This comment has been minimized.

codecov bot commented Nov 5, 2017

Codecov Report

Merging #668 into master will decrease coverage by 14.71%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #668       +/-   ##
===========================================
- Coverage   93.86%   79.14%   -14.72%     
===========================================
  Files          11       10        -1     
  Lines        2364     2369        +5     
===========================================
- Hits         2219     1875      -344     
- Misses        145      494      +349
Impacted Files Coverage Δ
tox/venv.py 89.65% <100%> (-6.61%) ⬇️
tox/config.py 95.33% <100%> (-2.25%) ⬇️
tox/session.py 54.37% <0%> (-39.97%) ⬇️
tox/_pytestplugin.py 69.07% <0%> (-24.9%) ⬇️
tox/interpreters.py 60.93% <0%> (-21.1%) ⬇️
tox/result.py 98.07% <0%> (-1.93%) ⬇️
tox/_quickstart.py 81.81% <0%> (-1.4%) ⬇️
tox/__main__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc34546...a58000d. Read the comment docs.

@asottile

This comment has been minimized.

Member

asottile commented Nov 5, 2017

You may be interested in tox-pip-extensions which attempts to solve this problem more generically (the venv-update extension diffs requirements and gets to a final state by installing/uninstalling)

@obestwalter

This comment has been minimized.

Member

obestwalter commented Nov 6, 2017

Hi @kapilt and @asottile if you both agree that this is a problem worth solving (I definitely also agree :)) and you both already have approaches it would be great if you could put your heads together and tackle that together. This really is something that comes up an awful lot and confuses users who assume that this works properly. I collected all tickets that have to do with that general problem here: https://github.com/tox-dev/tox/projects/9

def digest(self):
fname = str(self.name)
if fname.startswith('-r'):
fname = fname[2:]

This comment has been minimized.

@merwok

merwok Nov 10, 2017

Contributor

There may be optional whitespace: -r requirements.txt

This comment has been minimized.

@cryvate

cryvate Nov 11, 2017

Contributor

Actually, there can be at the moment, but it won't work as expected: it would search for requirements.txt (note space). See PR #669.

You should check for --requirement as well, in which case you will have to strip an =.

A requirements file itself can depend on a requirements file (e.g. dev requirements on install requirements). I guess that is not being checked here?

@gaborbernat

This comment has been minimized.

Member

gaborbernat commented Jun 9, 2018

In lack of activity I'll close this for now, we can reopen it when we come up wit ha common solution.

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