Fix rez-pip on Windows #1155
Fix rez-pip on Windows #1155
Conversation
There was a problem hiding this comment.
Just did a visual review as my two cents, also pinning @nerdvegas for enabling the GitHub workflow.
| default_pathext = \ | ||
| '.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC' | ||
| pathext = env.get("PATHEXT", default_pathext).split(os.pathsep) |
There was a problem hiding this comment.
Maybe we better keep the default_pathext, for backward compat.
|
|
||
| # Source: https://github.com/cookiecutter/whichcraft/blob/0.6.1/whichcraft.py | ||
| # Copyright (c) 2015-2016, Daniel Roy Greenfeld | ||
| # All rights reserved. | ||
|
|
There was a problem hiding this comment.
Not sure if this is okay, since Rez is going to be re-licensed under Apache2.0 (#1119) and the whichcraft is under BSD-3.
| if any(cmd.lower().endswith(ext.lower()) for ext in pathext): | ||
| files = [cmd] | ||
| else: | ||
| files = [cmd + ext for ext in pathext] |
There was a problem hiding this comment.
And just for consistency, ensure the ext is lowercase.
| files = [cmd + ext for ext in pathext] | |
| files = [cmd + ext.lower() for ext in pathext] |
| # On windows the system paths might have %systemroot% | ||
| name = os.path.expandvars(name) |
There was a problem hiding this comment.
We should better keep this, see 4bf62bf.
|
@davidlatwe I approved the workflows. |
|
Hi Manuel, Looks good, but there's a few things we need to do:
The best way to deal with this imo is to add whichcraft as a vendored lib (see others in vendor/). However, I would like to do this in a way that lets us easily see what modifications have been made also (unfortunately this wasn't done in existing vendored libs, live and learn). To that end, may I suggest:
This way, if we ever need to update the vendored lib while keeping our rez-specific changes, we have enough info to rebase on. Action items:
With this in mind let's leave this PR as-is and keep it for reference only, and I'll just close it after we've done all of the above. Thx |
|
See #1157 |
Resolves #904 , #1024
This is my first PR here, please let me know if there's more to add. :)
Intended change
This should now fix the issues with rez-pip on windows, which previously was not able to find python.
Implementation
The implementation by @ColinKennedy is basically taken from here and therefore based on another library called whichcraft. The library function is extended to also take an environment parameter.
The library is BSD licensed and I tried to include the relevant copyright notices.
Tested on
Windows 10
Rez 2.98
Python 3.9.7
Suggestions/feedback welcome!
Cheers,
Manuel