tox: fix environment setup #4657

benoit-pierre · 2017-08-08T22:56:21Z

Use virtualenv's pip for installing dependencies, not the code that is about to be tested.

benoit-pierre · 2017-08-08T23:08:55Z

Right... forgot pip's issue with self-updating on Windows. Amended to use python -I -m pip.

benoit-pierre · 2017-08-08T23:12:12Z

And python -I is not supported by older Python versions... sigh

benoit-pierre · 2017-08-08T23:46:05Z

OK, so now the issue is the un-vendored tox envs...

benoit-pierre · 2017-08-09T01:52:29Z

The un-vendored tests are a lie!

Because:

tox --notest is called, creating the environment
then we un-vendor pip (except the wheels just added to pip/_vendor are deleted too, so it would never have worked anyway)
and then tox is run again to run the tests, but the first thing tox does is install the code to be tested (pip) from sdist, so we're back to a regular vendored pip...

benoit-pierre · 2017-08-09T02:19:51Z

So I've reworked how un-vendored jobs are handled: tox now supports 2 dedicated environments (py27-novendor and py36-novendor). making it easier to use locally for testing. Un-vendoring is done by using a custom install_command that automatically un-vendors pip each time it's installed.

But there's still an issue stemming from the fact that a subsequent tox run will use the code we tested during the preceding run when re-installing pip's current code from sdist. So we're back to the problem this PR was trying to address... This was the reason the second tox run (for the integration tests) was failing on Travis: some entries were apparently missing for proper un-vendored support (see last commit).

Anyway, I see 2 ways to make sure that the installation is always done with a stable version of pip:

set recreate=True in tox.ini so the environment is re-created every time tox is run
instead of using pip/python -m pip, use a copy of get-pip.py

pradyunsg · 2017-08-09T11:44:23Z

instead of using pip/python -m pip, use a copy of get-pip.py

I would oppose such a change - it would make it difficult to run the tests without network, something I've had to do quite a few times.

pradyunsg · 2017-08-09T08:21:16Z

tox.ini

@@ -10,7 +10,16 @@ setenv =
    LC_CTYPE = en_US.UTF-8
 deps = -r{toxinidir}/dev-requirements.txt
 commands = py.test --timeout 300 []
-install_command = python -m pip install {opts} {packages}
+# Make sure the version of pip we're about to test is
+# not itself being used for setting up the environment.


I don't think this is a major issue? I've personally used this behaviour in hacky ways when working on different issues.

I find it completely abnormal to be using the code to be tested for setting up the environment itself, it should not be trusted.

Furthermore, if it fails during the tox env setup phase, instead of failing during the tests, the information you'll get from the install_command log are going to be less useful for identifying what went wrong then the results of a testsuite run.

And if the install_command does not return an error, that does not necessarily mean the environment was correctly setup: if a subtle bug was introduced in the code (like if suddenly pre-releases are being used when not asked for). How can you trust the testsuite then?

IMHO the whole process should be as reproducible as possible, and that means using a stable version of pip to set up the environment so the testsuite always as a chance to be run.

Okay - I see where you're coming from at this - I agree with you that using the code you want to test, to setup it's own test environment, is a little weird.

Changing the current setup to not do so is definitely an improvement. :)

pradyunsg · 2017-08-09T11:46:34Z

The un-vendored tests are a lie!

Oh wow. This is actually a valid issue and something that needs to be fixed. I didn't check this PR yet but I did verify that indeed the tests (in master) are running on a version of pip that bundles the vendored files.

benoit-pierre · 2017-08-09T11:58:07Z

instead of using pip/python -m pip, use a copy of get-pip.py

I would oppose such a change - it would make it difficult to run the tests without network, something I've had to do quite a few times.

Don't some of the tests need network access too? If using get-pip.py, you would only need access for setting up the tox environment, and the get-pip.py script could also be cached.

What about option 1, use recreate=True, instead? (Just to be clear, that's 2 different ways to solve the issue, both are not needed to solve it)

pradyunsg · 2017-08-09T12:16:50Z

If using get-pip.py, you would only need access for setting up the tox environment, and the get-pip.py script could also be cached.

Yeah - I thought about this but it just doesn't feel right.

Don't some of the tests need network access too?

They're all marked with the network and I just deselect them.

What about option 1, use recreate=True, instead?

Regenerating the tox venv when running the py**-unvendor tests sounds fine to me - not for the normal py** tests though. :)

benoit-pierre · 2017-08-09T12:26:17Z

What about option 1, use recreate=True, instead?

Regenerating the tox venv when running the py**-unvendor tests sounds fine to me - not for the normal py** tests though. :)

Why? This is about solving the original issue this PR was supposed to address. Re-creating the tox venv is not going to fix the problem with un-vendored jobs running a vendored version of pip

The only relation between the 2 issues is that if tox env's pip had been used, the fact that the short-lived un-vendored version was not working (because of the deleted wheels) would have been immediately detected. That's another good reason for making sure the tox env version is always used.

benoit-pierre · 2017-08-09T12:32:18Z

Just to be clear, this is what's happening for an un-vendored job on master:

tox --notest is run: pip (source dir) is used to install the tox env deps
tox's pip is incorrectly un-vendored: wheels are generated, copied to pip/_vendor, but deleted as part of removing the sources in pip/_vendor
tox is run again: again pip (source dir) is used to install the code being tested from sdist

The failure in 2. to properly un-vendor is not detected when running pip for 3. because the source dir version is used...

benoit-pierre · 2017-08-09T12:35:56Z

Anyway, I'll try to think of some way to fix the original issue without hitting the network.

In the mean time, how should I PR the un-vendoring problem? Do you want separate PR for 922f9a3 and 66d16ad? Only the later needs a news entry, I think. Do changes that are not user visible need a news entry?

pradyunsg · 2017-08-09T12:35:59Z

This is about solving the original issue this PR was supposed to address.

Oh, my bad. X|

Thanks for patiently explaining this stuff to me. :)

I'll come back to this PR a little later today...

pradyunsg · 2017-08-09T12:50:31Z

Anyway, I'll try to think of some way to fix the original issue without hitting the network.

Thank you!

Worth stating -- I'm by no means a strong -1 toward needing to hit the network for running tests (it's more of a -0.5). It's just that the network I use is slightly unreliable in nature (and it's not under my control) so I try to not get red colour on my terminal because of it. :P

Do you want separate PR for 922f9a3 and 66d16ad?

That would be nice. :)

Do changes that are not user visible need a news entry?

I think anything that makes a major change in the development workflow or is a user facing change warrants a news file. So, these would probably not need a news entry.

benoit-pierre · 2017-08-09T16:24:00Z

OK, so I've created #4661 for adding the required missing entries to pip._vendor (still part of this PR to see if the un-vendored tests pass).

And I have reworked how travis/tox's environment is setup and how un-vendored jobs are supported:

Use the virtualenv's pip for installing dependencies, not code that is about to be tested.
Add 2 dedicated un-vendored environments (py27-novendor and py36-novendor), making it easier to use locally for testing.
Un-vendoring is done by using a custom install_command that automatically un-vendors pip each time it's installed.

BrownTruck · 2017-08-09T23:15:01Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pradyunsg · 2017-08-09T16:27:20Z

tox.ini

@@ -2,6 +2,8 @@
 envlist =
    docs, packaging, lint-py2, lint-py3,
    py27, py33, py34, py35, py36, py37, pypy
+_pip = python {toxinidir}/.tox_helpers/runpip.py


I'd put these in a novendor section (like lint).

But _pip is used for other environments too. How about a dedicated [helpers] section then?

A [helpers] section with a nice comment documenting why all these things are done this way would be nice! ^.^

benoit-pierre · 2017-08-10T14:55:45Z

Rebased to avoid conflicts + tweaked comments in tox.ini.

dstufft · 2017-08-31T18:17:42Z

Probably we should just get rid of the unvendoring tests. Nobody can get an unvendored pip without patching pip, so presumably they can also run the tests after having patched it to see if something broke.

Since de-vendoring support exists only for downstream, and they need to patch pip to get that support anyways, it seems reasonable to push support for testing that configuration onto them. This is something they need to do anyways, since they need to test their versions of the vendored libraries. See pypa#4657 for more information.

pradyunsg · 2017-09-01T09:07:27Z

@dstufft I think this PR is still relevant. Could you take a look at it?

benoit-pierre · 2017-09-01T14:09:45Z

Rebased.

benoit-pierre · 2017-09-01T15:37:36Z

Not necessary anymore now that pip's source code does not live in the current directory.

pradyunsg · 2017-09-01T15:54:24Z

Ah, nice!

pradyunsg · 2017-09-02T11:19:44Z

Uhm... This is still an issue...

I'm still seeing that tox uses the development pip when installing stuff - which is what this PR is supposed to fix.

benoit-pierre · 2017-09-02T11:43:11Z

Yep, on the second run, for installing pip's code itself...

diff --git i/src/pip/__main__.py w/src/pip/__main__.py
index 4609582c..861ceee9 100644
--- i/src/pip/__main__.py
+++ w/src/pip/__main__.py
@@ -3,6 +3,8 @@ from __future__ import absolute_import
 import os
 import sys
 
+assert False
+
 # If we are running from a wheel, add the wheel to sys.path
 # This allows the usage python pip-*.whl/pip install pip-*.whl
 if __package__ == '':

> tox -e py36 --recreate --notest
GLOB sdist-make: setup.py
py36 create: .tox/py36
py36 installdeps: -rdev-requirements.txt
py36 inst: dist/pip-10.0.0.dev0.zip
py36 installed: apipkg==1.4,execnet==1.4.1,freezegun==0.3.9,mock==1.0.1,pretend==1.0.8,py==1.4.34,pytest==3.2.1,pytest-catchlog==1.2.2,pytest-forked==0.2,pytest-rerunfailures==3.1,pytest-timeout==1.2.0,pytest-xdist==1.20.0,python-dateutil==2.6.1,PyYAML==3.12,scripttest==1.3,six==1.10.0,virtualenv==15.2.0.dev0
_____________________________________________________________________________________________________ summary ______________________________________________________________________________________________________
  py36: skipped tests
  congratulations :)
> tox -e py36 --notest
GLOB sdist-make: setup.py
py36 inst-nodeps: pip-10.0.0.dev0.zip
ERROR: invocation failed (exit code 1), logfile: .tox/py36/log/py36-4.log
...

Use the virtualenv's pip for installing dependencies, not the code that is about to be tested. commit 922f9a3f6dc4ac642492204808b4d5cc995aca66 Author: Benoit Pierre <benoit.pierre@gmail.com> Date: Wed Aug 9 02:47:39 2017 +0200 tox/travis: fix and rework handling of un-vendored jobs

benoit-pierre · 2017-09-02T13:16:08Z

Tweaked pip launcher script.

pradyunsg · 2017-09-02T15:36:46Z

Appveyor doesn't seem very happy; any idea what happened there?

benoit-pierre · 2017-09-02T17:41:12Z

I don't know, for some reason somewhere in the bowels of pip, in pip.pip_vendor.distlib.resources, a check for os.path.exists(r'.tox\py\pip.tmp\pip\_vendor\distlib\t32.exe') fails, although the file does exist...

pradyunsg · 2017-09-02T17:52:05Z

This should have re-triggered the builds. If they still fail, it's something that'll need to be looked into.

benoit-pierre · 2017-09-02T17:56:38Z

There's no need to re-trigger the build. I can reproduce the issue on my Windows 10 VM. I don't know how to fix it, and I've already spent enough time on this, so I'm going to close this PR. Someone else will have to find a way to fix the initial issue (or just use recreate = True in tox.ini).

pradyunsg · 2017-09-02T17:58:51Z

Cool! Thanks a lot for looking into this @benoit-pierre! ^.^

pradyunsg · 2018-09-07T07:35:06Z

This approach breaks the build-isolation code. :/

lock · 2019-06-01T18:45:29Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

benoit-pierre force-pushed the fix_tox.ini_install_command branch from 23978dd to 32679e3 Compare August 8, 2017 23:08

benoit-pierre force-pushed the fix_tox.ini_install_command branch from 4ff0983 to c212ea2 Compare August 8, 2017 23:45

pradyunsg reviewed Aug 9, 2017

View reviewed changes

benoit-pierre force-pushed the fix_tox.ini_install_command branch 2 times, most recently from 66d16ad to 6f58925 Compare August 9, 2017 16:19

benoit-pierre changed the title ~~tox: fix install_command~~ tox/travis: fix environment setup and un-vendored support Aug 9, 2017

BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 9, 2017

pradyunsg reviewed Aug 10, 2017

View reviewed changes

benoit-pierre force-pushed the fix_tox.ini_install_command branch from 6f58925 to 6080ff2 Compare August 10, 2017 14:53

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 10, 2017

benoit-pierre force-pushed the fix_tox.ini_install_command branch from 6080ff2 to 7c0873e Compare August 10, 2017 14:55

pradyunsg added C: automation Automated checks, CI etc type: maintenance Related to Development and Maintenance Processes labels Aug 10, 2017

benoit-pierre force-pushed the fix_tox.ini_install_command branch from bab0ea7 to 6bd519f Compare September 1, 2017 14:09

benoit-pierre closed this Sep 1, 2017

benoit-pierre reopened this Sep 2, 2017

benoit-pierre force-pushed the fix_tox.ini_install_command branch from 6bd519f to a514354 Compare September 2, 2017 13:15

pradyunsg closed this Sep 2, 2017

pradyunsg reopened this Sep 2, 2017

benoit-pierre closed this Sep 2, 2017

benoit-pierre mentioned this pull request Jul 30, 2018

Don't install development tree for non-test tox jobs #5673

Merged

pradyunsg mentioned this pull request Sep 7, 2018

Use a helper to ensure tox always runs a known-good pip version #5770

Closed

benoit-pierre mentioned this pull request Oct 2, 2018

tox: fix environment setup #5843

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019

tox: fix environment setup #4657

tox: fix environment setup #4657

Conversation

benoit-pierre commented Aug 8, 2017

benoit-pierre commented Aug 8, 2017

benoit-pierre commented Aug 8, 2017

benoit-pierre commented Aug 8, 2017

benoit-pierre commented Aug 9, 2017

benoit-pierre commented Aug 9, 2017

pradyunsg commented Aug 9, 2017

pradyunsg Aug 9, 2017

Choose a reason for hiding this comment

benoit-pierre Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

pradyunsg Aug 9, 2017

Choose a reason for hiding this comment

pradyunsg commented Aug 9, 2017 • edited Loading

benoit-pierre commented Aug 9, 2017

pradyunsg commented Aug 9, 2017

benoit-pierre commented Aug 9, 2017

benoit-pierre commented Aug 9, 2017

benoit-pierre commented Aug 9, 2017

pradyunsg commented Aug 9, 2017

pradyunsg commented Aug 9, 2017 • edited Loading

benoit-pierre commented Aug 9, 2017

BrownTruck commented Aug 9, 2017

pradyunsg Aug 9, 2017

Choose a reason for hiding this comment

benoit-pierre Aug 10, 2017

Choose a reason for hiding this comment

pradyunsg Aug 10, 2017

Choose a reason for hiding this comment

benoit-pierre Aug 10, 2017

Choose a reason for hiding this comment

benoit-pierre commented Aug 10, 2017

dstufft commented Aug 31, 2017

pradyunsg commented Sep 1, 2017

benoit-pierre commented Sep 1, 2017

benoit-pierre commented Sep 1, 2017

pradyunsg commented Sep 1, 2017

pradyunsg commented Sep 2, 2017

benoit-pierre commented Sep 2, 2017

benoit-pierre commented Sep 2, 2017

pradyunsg commented Sep 2, 2017

benoit-pierre commented Sep 2, 2017

pradyunsg commented Sep 2, 2017

benoit-pierre commented Sep 2, 2017

pradyunsg commented Sep 2, 2017

pradyunsg commented Sep 7, 2018

lock bot commented Jun 1, 2019

benoit-pierre Aug 9, 2017 •

edited

Loading

pradyunsg commented Aug 9, 2017 •

edited

Loading

pradyunsg commented Aug 9, 2017 •

edited

Loading