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

tox -e lint doesn't work from behind a proxy #7599

Closed
pfmoore opened this issue Jan 14, 2020 · 11 comments · Fixed by #7602
Closed

tox -e lint doesn't work from behind a proxy #7599

pfmoore opened this issue Jan 14, 2020 · 11 comments · Fixed by #7602
Labels
auto-locked Outdated issues that have been locked by automation C: proxy Dealing with proxies and networking type: maintenance Related to Development and Maintenance Processes

Comments

@pfmoore
Copy link
Member

pfmoore commented Jan 14, 2020

Environment

  • pip version: 19.3.1
  • Python version: 3.8.0
  • OS: Windows 10

Running tox 3.14.4, pre-commit 1.21.0

Description
I'm running behind a proxy with environment variables http{s}_proxy set. When I run tox -e lint a git call in pre-commit fails with a network error. Investigation shows that the git command is run in an environment without my proxy variables.

This is almost certainly an issue with tox or pre-commit, and not directly with pip. But I don't have time right now to debug, and I wanted to record the problem somewhere. My main development machine doesn't need a proxy, so this is not critical for me, but I'll look into it more when I get the chance.

Expected behavior
Proxy settings are passed to git.

How to Reproduce
See above

Output

>.\.venv\Scripts\tox.exe -e lint
lint create: C:\Work\Projects\pip\.tox\lint
lint installdeps: pre-commit
lint installed: aspy.yaml==1.3.0,cfgv==2.0.1,identify==1.4.9,nodeenv==1.3.4,pip==19.3.1,pre-commit==1.21.0,PyYAML==5.3,setuptools==42.0.2,six==1.13.0,toml==0.10.0,virtualenv==16.7.9,wheel==0.33.6
lint run-test-pre: PYTHONHASHSEED='191'
lint run-test: commands[0] | pre-commit run --all-files --show-diff-on-failure
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
An unexpected error has occurred: CalledProcessError: command: ('C:\\Program Files\\Git\\cmd\\git.exe', 'fetch', 'origin', '--tags')
return code: 128
expected return code: 0
stdout: (none)
stderr:
    fatal: unable to access 'https://github.com/pre-commit/pre-commit-hooks/': Failed to connect to github.com port 443: Timed out

Check the log at C:\Users\UK03306/.cache\pre-commit\pre-commit.log
ERROR: InvocationError for command 'C:\Work\Projects\pip\.tox\lint\Scripts\pre-commit.EXE' run --all-files --show-diff-on-failure (exited with code 1)
_______________________________________________________________________ summary _______________________________________________________________________ ERROR:   lint: commands failed
@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Jan 14, 2020
@pfmoore
Copy link
Member Author

pfmoore commented Jan 14, 2020

Adding HTTP_PROXY and HTTPS_PROXY to the passenv setting in tox.ini seems to work. That sucks a bit, but blame tox for that, I guess :-(

@pradyunsg
Copy link
Member

What about nox -s lint? ;)

@pradyunsg pradyunsg added C: proxy Dealing with proxies and networking type: maintenance Related to Development and Maintenance Processes labels Jan 14, 2020
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Jan 14, 2020
@pfmoore
Copy link
Member Author

pfmoore commented Jan 15, 2020

Yep, that worked :-)

The docs say to use tox. Is that something that should change, or is using nox "unofficial" for now?

The nox version doesn't check uncommitted changes, which sucks a bit. Looks like running via tox does pick these up, so I guess tox is "better".

@pradyunsg
Copy link
Member

pradyunsg commented Jan 15, 2020

nox is unofficial for now, since I reckon we'd want to have the ability to invoke tests with arbitrary interpreters.

The commands being run are the exact same in tox and nox, so I'm not sure what difference you're seeing (and there's not enough context to make out what might be the cause - note that pre-commit gets the list of files from git, so if git doesn't track a file, pre-commit checks don't run on it).

Once nox can be invoked with arbitrary interpreters, we should go ahead and drop tox entirely from our tool chain IMO.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 15, 2020

The commands being run are the exact same in tox and nox, so I'm not sure what difference you're seeing

Yeah, I don't really know what happened myself. I'm on my slow laptop at the moment and runtimes of 10 minutes or more don't lend themselves to iteratively checking what's going on :-( I'll try to do some proper investigation (of both this and the test suite - I got lots of failures when I "just ran" the test suite, but I think it's because on Windows we need some extra arguments to skip tests that don't work there, and I don't recall what those args are right now) when I switch to the good laptop later. But this is why I've previously tended to let CI do the work for me - if I'm going to have to leave things to run in the background, I might as well do so on someone else's server :-)

@pradyunsg
Copy link
Member

Oh, if additional arguments are needed to run pip's tests out of the box, that's not right.

@uranusjr
Copy link
Member

Tox does not pass all environment variables when it runs commands. Nox OTOH passes all environment variables by default. That would explain why you need to set passenv explicitly in Tox, but Nox works out of the box.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 15, 2020

Yeah, my feeling is that this reflects the difference in design focus of the 2 tools. Both are technically for running tests, but nox feels more general-purpose. Protecting tests from being messed up by environment settings seems like a good goal. Conversely, not having automations break because they rely on user-specific config via environment settings also seems like a good idea.

Maybe we should have HTTP{S}_PROXY in our PASSENV, though. I can't imagine any situation where a user would say "network access needs to go through this proxy" and tox should say "nope, I don't believe you" ;-) So technically tox should pass these by default, but let's fix our config for now.

I'll create a PR for that.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 15, 2020

Actually, I also raised tox-dev/tox#1498

@pradyunsg
Copy link
Member

I agree that we should fix our tox configuration.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 16, 2020

Oh, if additional arguments are needed to run pip's tests out of the box, that's not right.

The commands to run the tests on Windows (from the CI config) are:

tox -e py -- -m unit -n auto
tox -e py -- -m integration -n auto

Those work. But just tox -e py produces loads of errors, suggesting that there are tests that are marked as neither "unit" nor "integration" that are failing on Windows. Worse, after running the incorrect command, I found masses of unexpected packages installed in my system Python! (I was running tox from a venv created just for testing pip, so everything should have been 100% isolated from the system install. It was bad enough that reinstalling my system Python was the easiest fix.

I'll make this comment into a separate issue, but honestly I don't want to spend a lot of time fighting the test suite into submission on Windows, particularly if it involves trashing my system Python on a regular basis 🙁 So if anyone else wants to pick this up, they are welcome to do so.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: proxy Dealing with proxies and networking type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants