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

recreate venv when -rrequire.txt changes #668

Closed
wants to merge 10 commits into from
Closed

Conversation

kapilt
Copy link

@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
Copy link

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
Copy link
Contributor

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
Copy link
Member

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:]
Copy link

Choose a reason for hiding this comment

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

There may be optional whitespace: -r requirements.txt

Copy link

Choose a reason for hiding this comment

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

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 gaborbernat added needs:review somebody who thinks they know what they are doing should have a look at this needs:work for PRs: not quite there and needs some changes labels Dec 1, 2017
@gaborbernat
Copy link
Member

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

@gaborbernat gaborbernat closed this Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:review somebody who thinks they know what they are doing should have a look at this needs:work for PRs: not quite there and needs some changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants