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

Follow up to closed PR #1677 #1681

Closed
wants to merge 3 commits into from
Closed

Conversation

techtonik
Copy link
Contributor

See #1677 for discussion. #1677 was closed and doesn't update itself with new commits anymore.

I am not sure how to detect that pip is run as pip.exe
The code above manages to self-update when executed as Scripts\pip install pip==1.5.2
@Ivoz
Copy link
Contributor

Ivoz commented Mar 25, 2014

What concept is this proving exactly? I thought the issue was to have pip.exe overwrite itself, essentially by calling python -m pip install -U pip in a separate process and closing the one in which pip.exe already exists, but which is apparently non-trivial to do on Windows. But.. this just installs the pypi package wget?

@pfmoore
Copy link
Member

pfmoore commented Mar 25, 2014

This is not a working patch to pip that fixes the issue described. Please stop submitting "proof of concept" and incomplete patches. If you can make it so "pip install -U pip" works without an error about being unable to overwrite pip.exe then please do so, otherwise stop wasting people's time. Nobody else is going to write the patch, no matter how often you tell us it's easy.

@techtonik
Copy link
Contributor Author

@pfmoore and @Ivoz - have you tried to execute pip install -U pip with this patch? What was your result, please.

@techtonik
Copy link
Contributor Author

If that's not obvious this patch proves that pip.exe is replaced. It is what you need to check. @pfmoore has wrong assumption that it doesn't happen, but do not want to execute the script that proves otherwise. I don't know how to persuade him.

@techtonik
Copy link
Contributor Author

@pfmoore, I am opening issues for a clear reason - to keep track for non-resolved problems and not to annoy you. It will save me some time if you wait for a few days before closing them,

@pfmoore
Copy link
Member

pfmoore commented Mar 26, 2014

@techtonik this is a pull request, not an issue. It needs to actually fix the issue in a way that can be included in pip and released. Hacking in a quick call to os.execv and asking us to test it is not what I was asking for. I can test this, but even if I say that it works, you will still need to create a complete patch using this technique. Presumably, you won't gain anything from reports that it works, as you are already convinced that it will. So why don't you spend your time writing a complete patch now, and then if it does work I can review it properly with the expectation that it will be commited?

@techtonik
Copy link
Contributor Author

@pfmoore - it is your words that #1299 needs code to go on. I can attach it there, but it is hard to update. I am telling you that pip install -U pip works without errors, which is contrary to your argument. You are neither withdrawing your argument, nor proving it, nor want to execute the code that proves you are wrong (or not - depending on your result).

Until you test this patch and confirm that proof of concept works, I won't invest more time in polishing it, because I don't want to waste time on something that won't be merged. It may happen that you have different Windoze and that's why it failed previously.

@pfmoore
Copy link
Member

pfmoore commented Mar 26, 2014

  1. Your patch is missing an import of "sys". Please provide patches that you have actually tested yourself.
  2. When I run this version, the command prompt returns immediately and pip's output is then written to the console while I am trying to execute a subsequent command.
  3. The "Access is denied" error still occurs.

So this approach does not work, as I repeatedly stated previously.

@techtonik
Copy link
Contributor Author

  1. I tested this patch on stable version that is installable from PyPI - it has the sys import.
  2. Command prompt returns immediately, yes, but that's another problem for now.
  3. If there is Access is denied error, there should be an error log. On my system it works ok. Can you remove your pip.exe under a user account?

I can't update this PR, because it is closed, so here is the correct code that should work with pip install -U pip (previous version was tested with pip install pip==1.5.2).

import sys
if not 'wget' in args:
    os.execv(sys.executable, [sys.executable, '-m', 'pip', 'install', '-U', 'wget'] + args)

@Ivoz Ivoz reopened this Mar 26, 2014
@pfmoore
Copy link
Member

pfmoore commented Mar 26, 2014

This won't alter the fact that the console regains control before pip writes its output. I will not be spending any more time on this, as I think I have proved to my satisfaction that this approach is unworkable.

@Ivoz
Copy link
Contributor

Ivoz commented Mar 26, 2014

I've re-opened for now. Feel free to:

  • rebase this on latest pip develop if necessary
  • Commit working code that will run as-is from your branch
  • When running pip.exe install --upgrade --force-reinstall pip will result in a newer pip.exe on a Windows system
  • Actually test & run with your branch to confirm that the above point is true

And comment here when the above are done so we can test and confirm. I can boot up into Windows to do so at some point (when the above are done) if you don't wish to handle it for the time being @pfmoore

@techtonik
Copy link
Contributor Author

@pfmoore, I guess I can read it as "I was wrong and I accept the fact that pip.exe can be rewritten correctly with os.execv call". So far it was the primary argument against. Now that this is no more an argument, it is possible to concentrate on part 2: console regains control before pip writes the output.

@duly
Copy link

duly commented Mar 28, 2014

@techtonik Could you please remove the extraneous comments and remove 'wget' so that this patch is workable?

@dstufft
Copy link
Member

dstufft commented Aug 30, 2014

I'm going to close this PR. It was a proof of concept which has shown it's concept. If someone wants to fix the issues exposed in this and make a proper PR for it they are welcome to.

@dstufft dstufft closed this Aug 30, 2014
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants