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

install --timeout ignored #70

Closed
vbabiy opened this Issue Mar 15, 2011 · 14 comments

Comments

Projects
None yet
7 participants
@vbabiy
Contributor

vbabiy commented Mar 15, 2011

I am attempting to use pip to install Traits, but it fails with a url timeout error. easy_install will install Traits, but it takes about 30 seconds after the command is issued before the download begins. So I tried running "pip install --timeout=60 Traits", but it raised the same error after about 15 seconds. Then I tried "pip install --timeout=1 Traits" and it timed out after 15 seconds again.

I don't think this option is being respected.


@ogirardot

This comment has been minimized.

Show comment
Hide comment
@ogirardot

ogirardot Dec 10, 2011

Contributor

+1, the timeout parameter doesn't seem to be passed on, working on it.

Contributor

ogirardot commented Dec 10, 2011

+1, the timeout parameter doesn't seem to be passed on, working on it.

ogirardot added a commit to ogirardot/pip that referenced this issue Dec 10, 2011

@ogirardot

This comment has been minimized.

Show comment
Hide comment
@ogirardot

ogirardot Dec 10, 2011

Contributor

pull request opened to fix this issue. The problem was that default initial options were overring any value specified.

Contributor

ogirardot commented Dec 10, 2011

pull request opened to fix this issue. The problem was that default initial options were overring any value specified.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Dec 12, 2011

Contributor

Unfortunately that fix isn't correct, it just replaces one problem with another one. The basic problem is that the line changed in your commit is fundamentally incorrect for an option with a non-False default (such as --timeout which has a default of 15). Because that default value will always show up either in the initial_options or the command options, and so either way we switch the priority, the default value might wrongly override a supplied value. Right now if you do pip install --timeout 1 foo the timeout option seems to have no effect. With your fix, if you do pip --timeout 1 install foo, it will have no effect.

I do think the precedence you switched it to is slightly better, in that if you (for some reason) do pip --timeout 1 install --timeout 3 foo, the latter option ought to take precedence; right now it doesn't, with your fix it would.

But it doesn't solve the core problem - we need a better way of handling these options other than looking for them twice with two separate parsers and then naively trying to merge the two sets of options with the wrong assumption that an option's default value will always be falsy.

The easiest answer would be if we could tell the global parser to parse the whole arglist (instead of just up to the first non-option), but ignore any bad options it finds (so that the command's option parser can still find them). Then the command option parsers wouldn't need to look for global options at all, and there would be no need for merging options. Unfortunately optparse provides no way to say "ignore bad options", and it seems like making it do so in a subclass would involve extensive surgery with internals.

So I'm not sure the best way to address this yet; need to think on it more.

Contributor

carljm commented Dec 12, 2011

Unfortunately that fix isn't correct, it just replaces one problem with another one. The basic problem is that the line changed in your commit is fundamentally incorrect for an option with a non-False default (such as --timeout which has a default of 15). Because that default value will always show up either in the initial_options or the command options, and so either way we switch the priority, the default value might wrongly override a supplied value. Right now if you do pip install --timeout 1 foo the timeout option seems to have no effect. With your fix, if you do pip --timeout 1 install foo, it will have no effect.

I do think the precedence you switched it to is slightly better, in that if you (for some reason) do pip --timeout 1 install --timeout 3 foo, the latter option ought to take precedence; right now it doesn't, with your fix it would.

But it doesn't solve the core problem - we need a better way of handling these options other than looking for them twice with two separate parsers and then naively trying to merge the two sets of options with the wrong assumption that an option's default value will always be falsy.

The easiest answer would be if we could tell the global parser to parse the whole arglist (instead of just up to the first non-option), but ignore any bad options it finds (so that the command's option parser can still find them). Then the command option parsers wouldn't need to look for global options at all, and there would be no need for merging options. Unfortunately optparse provides no way to say "ignore bad options", and it seems like making it do so in a subclass would involve extensive surgery with internals.

So I'm not sure the best way to address this yet; need to think on it more.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Dec 12, 2011

Contributor

One super-simple option would be to simply require that global options must be supplied before the command name. That would simplify the code, remove the need for merging options at all, and fix this bug. But it might not be an acceptable restriction.

@jezdez you've worked more with the config system, any thoughts on what would be an acceptable solution to this?

Contributor

carljm commented Dec 12, 2011

One super-simple option would be to simply require that global options must be supplied before the command name. That would simplify the code, remove the need for merging options at all, and fix this bug. But it might not be an acceptable restriction.

@jezdez you've worked more with the config system, any thoughts on what would be an acceptable solution to this?

@ogirardot

This comment has been minimized.

Show comment
Hide comment
@ogirardot

ogirardot Dec 12, 2011

Contributor

i agree with you on the latest point, it's a counter intuitive restriction to only allow "pip --timeout 1 instal ..."
and not exactly widespread in the command-line world.

I understand now why this patch isn't correct, that also means my test suite isn't enough, i'll add the proper tests when i've got the time.

Contributor

ogirardot commented Dec 12, 2011

i agree with you on the latest point, it's a counter intuitive restriction to only allow "pip --timeout 1 instal ..."
and not exactly widespread in the command-line world.

I understand now why this patch isn't correct, that also means my test suite isn't enough, i'll add the proper tests when i've got the time.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Dec 12, 2011

Contributor

Speaking of that test, I think tests for this option-parsing issue need to be a bit broader in scope, not unit tests of just BaseCommand, because the issue is really an interaction of the base parser (used in pip.main()) and the BaseCommand parser (and merge_options() method), it's not just the latter alone. The tricky thing is figuring out how to run pip.main() and still intercept option values at a useful point (because it's almost impossible to detect that the timeout option was set correctly based just on the output of a subprocess pip run). Don't have any immediate good ideas here either :/ but i'll take another look at it when I'm more awake.

Contributor

carljm commented Dec 12, 2011

Speaking of that test, I think tests for this option-parsing issue need to be a bit broader in scope, not unit tests of just BaseCommand, because the issue is really an interaction of the base parser (used in pip.main()) and the BaseCommand parser (and merge_options() method), it's not just the latter alone. The tricky thing is figuring out how to run pip.main() and still intercept option values at a useful point (because it's almost impossible to detect that the timeout option was set correctly based just on the output of a subprocess pip run). Don't have any immediate good ideas here either :/ but i'll take another look at it when I'm more awake.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Dec 12, 2011

Contributor

(Also, note that there's already a tests/test_config.py test file, and option-related tests should probably go in there.)

Contributor

carljm commented Dec 12, 2011

(Also, note that there's already a tests/test_config.py test file, and option-related tests should probably go in there.)

@ogirardot

This comment has been minimized.

Show comment
Hide comment
@ogirardot

ogirardot Dec 12, 2011

Contributor

maybe the right work here would be precisely to separate the parser part from the BaseCommand class in order for it to be more testable (as an independent part of the system).

Contributor

ogirardot commented Dec 12, 2011

maybe the right work here would be precisely to separate the parser part from the BaseCommand class in order for it to be more testable (as an independent part of the system).

@pysqz

This comment has been minimized.

Show comment
Hide comment
@pysqz

pysqz Mar 8, 2013

"--log-file" is also ignored. When I run pip with '--log-file /tmp/X/X.log', the value can't be changed, it's always "~/.pip/pip.log"

pysqz commented Mar 8, 2013

"--log-file" is also ignored. When I run pip with '--log-file /tmp/X/X.log', the value can't be changed, it's always "~/.pip/pip.log"

pysqz pushed a commit to pysqz/pip that referenced this issue Mar 8, 2013

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 8, 2013

Contributor

@gvalkov , interested in this? read the history above. I haven't looked at this close.
(btw, getting to you other stuff soon)

Contributor

qwcode commented Mar 8, 2013

@gvalkov , interested in this? read the history above. I haven't looked at this close.
(btw, getting to you other stuff soon)

@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 9, 2013

Contributor

Sure. Since sub-parsers actually know of the options in the 'Generic Options' option group, why not get rid of merge_options() and let sub-commands parse 'General Options' themselves?

Currently __init__.parseopts() returns:

parseopts('pip -v --timeout 25 install --timeout 30 INITools') 
=> 'install', <options>, '--timeout 30 INITools', <parser>

Let's just make it pass all arguments to the command parser:

parseopts('pip -v --timeout 25 install --timeout 30 INITools')
=> 'install', <options>, '-v --timeout 25 --timeout 30 INITools', <parser>
command.main('-v --timeout 25 --timeout 30 INITools', ...)

Here's a quick jab at the problem (tests pass). Tell me if you like this approach and I'll polish it.

Contributor

gvalkov commented Mar 9, 2013

Sure. Since sub-parsers actually know of the options in the 'Generic Options' option group, why not get rid of merge_options() and let sub-commands parse 'General Options' themselves?

Currently __init__.parseopts() returns:

parseopts('pip -v --timeout 25 install --timeout 30 INITools') 
=> 'install', <options>, '--timeout 30 INITools', <parser>

Let's just make it pass all arguments to the command parser:

parseopts('pip -v --timeout 25 install --timeout 30 INITools')
=> 'install', <options>, '-v --timeout 25 --timeout 30 INITools', <parser>
command.main('-v --timeout 25 --timeout 30 INITools', ...)

Here's a quick jab at the problem (tests pass). Tell me if you like this approach and I'll polish it.

@hltbra

This comment has been minimized.

Show comment
Hide comment
@hltbra

hltbra Apr 19, 2013

Member

Hi @gvalkov. I think it is the way to go. There are some bugs reported because of merge_options(). Are you going to work on that? I would like to merge it asap :-)

Member

hltbra commented Apr 19, 2013

Hi @gvalkov. I think it is the way to go. There are some bugs reported because of merge_options(). Are you going to work on that? I would like to merge it asap :-)

@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Apr 19, 2013

Contributor

Sure. Here's a commit that you can cherry-pick and a test build. Can send a pull request later if necessary.

Contributor

gvalkov commented Apr 19, 2013

Sure. Here's a commit that you can cherry-pick and a test build. Can send a pull request later if necessary.

hltbra added a commit to hltbra/pip that referenced this issue Apr 28, 2013

Fix pypa#70 and pypa#350: options were messed up because of Command.m…
…erge_options()

Issue pypa#70: install --timeout ignored and
Issue pypa#350: Pip lies about log location
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Sep 17, 2013

Contributor

fixed in PR #1202

Contributor

qwcode commented Sep 17, 2013

fixed in PR #1202

@qwcode qwcode closed this Sep 17, 2013

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