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

[Fix] Windows rez-bind pip #659

Merged
merged 6 commits into from
Jul 20, 2019
Merged

[Fix] Windows rez-bind pip #659

merged 6 commits into from
Jul 20, 2019

Conversation

lambdaclan
Copy link
Contributor

@lambdaclan lambdaclan commented Jul 8, 2019

Description

[Depends on PR #602, #654. #656]

Currently running rez-bind pip will create a broken package unless you run it with admin privileges. This has to do with symlinking pip, and as you can't do that on non-administrative accounts there are things missing from the package rendering it in a broken state. This can potentially cause the pip version check to fail because the pip exe will be set to the broken package.

rez-bind pip on Windows causes an invalid pip package to be generated. I am not sure where the
mysterious 1.5.6 version comes from but is definitively not the same one as the one installed on the system.

pip_bind_fail

Now the version check correctly picks up the wrong version and raises an error but it actually hides the fact that the system pip version might be >= 19, it also hides the original exception

pip_bind_fail2

Saying that, the VersionError exception might prompt the user towards the right direction but is still
confusing nonetheless.

Remarks:

  • Windows 10 users please try and let me know if at all possible
  • Changes not final - still debugging
  • I will wait for some feedback from Windows users

Extra info

Linux:

Binding pip into /home/user/packages...
/home/user/.pyenv/versions/2.7.16/lib/python2.7/site-packages/pip OK
/home/user/packages/pip/19.1.1/platform-linux/arch-x86_64/os-Arch-rolling/python-2.7/python ??

Windows:

Binding pip into C:\Users\Joe\packages...
c:\users\joe\.rez\lib\site-packages\pip ???
C:\Users\Joe\packages\pip\1.5.6\platform-windows\arch-AMD64\os-windows-10.0.17763\python-2.7\python OK

On Linux rez correctly copies the pip directory tree from the Python that was used to install rez with
whereas on Windows, the directory tree that gets copied is quite different, it comes from the
lib directory of the rez itself?? That is where the weird 1.5.6 version seems to be coming from.

Update:

After further investigation, I realized that at least on my Windows 10 machine, every time pip is referenced it points to the wrong one under lib which has the version 1.5.6 causing the pip check test to always fail. The root of the issue seems to be there. This happens with both admin and non
admin privileges. This seems to be the fact of not using shell=True when running the python commands to retrieve version and path for pip.

I have managed to circumvent the issue during the bind that now also works without admin privileges
(only on Windows 10) by modifying the way the path and version to pip is retrieved. I would still like to know the reason pip has these issues so I will keep on looking.

pip now uses the correct path and displays the correct version

pip_check_correct

pip with the correct path and version still raises the VersionError (uses lib one)

pip_check_incorrect

pip package correctly created without admin privileges

pip_env_correct

Results

correct install using pip package (non admin)

correct_package

correct install using pip system (non admin)

correct_system

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

The fixes for these issues have been tested with various packages using the following:

Test Configuration:

  • Python version: 2.7.15, 2.7.16
  • OS: Linux, Windows 10
  • Toolchain: rez 2.33, pip 18.1, 19.1.1, distlib 0.2.9.post0

@lambdaclan lambdaclan mentioned this pull request Jul 8, 2019
6 tasks
@nerdvegas
Copy link
Contributor

Hey @lambdaclan,

It seems github isn't recalculating the diffs after I've merged #602 to master. Ala isaacs/github#750. There are so many diffs I can't make out which ones are specific to this PR.

Is there any chance you could re-merge with new master, or try what that thread suggests and change the base branch, then back again?

I'm also having the same issue with #628 :/

Apologies for the confusion.
A

@lambdaclan
Copy link
Contributor Author

Hello @nerdvegas

Thank you for merging #656! I have now merged the changes from master so only the relevant diffs are now visible, hope it is better now.

shutil.copytree(srcpath, os.path.join(destpath, name))
if name == "pip":
path = subprocess.check_output('python -c "import pip; print(pip.__path__)"', shell=True)
# retrieve second line since first is the Python 2 deprecation warning
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems too fragile, and likely to break after the py3 port. I'd suggest:

  • see if using shell=True across the board works. If so do that, rather than special-casing pip;
  • see if simply taking the last line of stdout works (rather than assuming there are >1 lines and that line[0] is a deprecation warning). Again this would remove the need for special-casing for pip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Allan sounds good to me,

see if using shell=True across the board works. If so do that, rather than special-casing pip;

This is what I originally had in mind but the warning about using shell=True on the documentation website and many other posts made me a bit sceptical. I will do the proposed fixes and report back shortly.

Maybe is worth adding shell=True across the board only if Windows is detected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nerdvegas Using shell=True across the board still works as expected on Windows. Saying that I noticed a significant time increase during the binding Python part on Linux. I believe it might be worth adding the platform check and enabling the flag only on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's slow, because on Linux arguments can't be passed as list when used with shell=True. You can " ".join(args) prior to passing it, on either platform, and that should fix it. On Windows, shell also exposes the local PATH, which shell=False doesn't, which has a slightly different behavior than Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Marcus, seems to be alright now. I have settled for a platform check instead I believe is cleaner that way. shell=True is only ever required on Windows because of the different behaviour you describe above.

Eventually we will need to refactor the code a bit to use the new subprocess.run instead of the old Popen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mottosso If you can give it a spin by the way and let me know if it works it would be great! It works on my Windows 10 VM. Should work without admin rights console.

Copy link
Contributor

@mottosso mottosso Jul 16, 2019

Choose a reason for hiding this comment

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

I currently doesn't work for me, as I'm on Python 3. Personally I wouldn't rely on the deprecation message being consistent enough to rely on in this way. How about printing something additional alongside the path, something to identify the output as yours? E.g:

print("my_unique_id: %s" % pip.__path__)

And then..

for line in out:
  if line.startswith("my_unique_id"):
    path = out.split("my_unique_id: ", 1)[-1]
    break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mottosso Hello Marcus, please try the latest version. The code has changed. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks good to me. If you add universal_newlines=True you'll also get consistent output between Python 2 and 3, but it shouldn't affect the behaviour under Python 2. 👍

@nerdvegas
Copy link
Contributor

nerdvegas commented Jul 16, 2019 via email

@lambdaclan
Copy link
Contributor Author

@nerdvegas Hello Allan, I did the changes as discussed so this should be OK for merge if you want.

@nerdvegas
Copy link
Contributor

Will get to this soon, thx

@nerdvegas nerdvegas merged commit 5d215bd into AcademySoftwareFoundation:master Jul 20, 2019
@lambdaclan lambdaclan deleted the fix/windows-bind-pip branch April 6, 2020 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants