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

[build_scripts] fix find_executable for windows .bat (ie, pyside2-uic.bat) #1454

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Feb 16, 2021

Description of Change(s)

There was a known issue with distutils.spawn.find_executable - that it fails to find .bat files (it only looks for .exe):
https://bugs.python.org/issue2200

I noticed this problem because the executable for pyside2-uic that got installed on my system is pyside2-uic.bat - presumably this is the same for many other windows users.

Unfortunately, since distutils is now deprecated, this will never be fixed, so just made a fixed version in build.py and renamed it FindExecutable.

For comparison, original code for find_executable:
https://github.com/python/cpython/blob/master/Lib/distutils/spawn.py#L91

Fixes Issue(s)

  • Building with pyside2 on windows (when pyside2-uic.bat is created)

@jilliene
Copy link

Filed as internal issue #USD-6574

@c64kernal c64kernal added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Feb 23, 2021
@c64kernal
Copy link
Contributor

Thanks for this, @pmolodo -- we'd love to not support this in the long term either. What do you think of detecting whether or not we are running Python 3.x and then using shutil.which? That way maybe this PR can be a lot smaller in that case, and the downside is that this would only be fixed on Windows in Python 3.x, but maybe that's okay for your use-case? Bonus points if we can detect the version of pyside installed and warn when the error condition would be hit? Just a thought.

@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 24, 2021

That could work - I am using python 3 for windows builds, so selfishly, would be fine for me.

As for checking for error condition - I'm not sure I would trust that a .bat is always created for pyside2, and not for (original) pyside. This certainly wouldn't be the first time that it happens that my computer is special. We could just always print a warning if we fail to find something on windows + python2.

Still a problem for people who want python-2 on windows, but... one which will go away, eventually...

Only other objection I could think of would be the one mentioned here, which is that this uses PATHEXT, which is that the actual function used, spawnv, doesn't support everything in PATHEXT. However... when I tested on my machine, all the "extra stuff" came after the standard ones spawnv does accept, so... it's probably not really an issue for us.

So... I'd be on board with using shutil.which...

@c64kernal
Copy link
Contributor

Okay thanks @pmolodo -- maybe best not to overthink this since this is the only report we've gotten of this problem (as far as I know) so if you only need Python 3 on Windows and shutil.which is the way of the future, let's switch to that for that platform at least for now until we can characterize further problems better :)

...if possible (ony available on python-3)

shutil.which is preferred, because it deals with additional windows
executable extensions, besides just .exe (ie, .bat - so
pyside2-uic.bat will be found)

See: https://bugs.python.org/issue2200

Also, distutils is deprecated
@pmolodo pmolodo force-pushed the pr/fix_find_executable_windows_bat branch from 83c8ba2 to 99c7013 Compare April 8, 2021 20:38
@pmolodo
Copy link
Contributor Author

pmolodo commented Apr 8, 2021

Pushed the change to use shutil.which - sorry to take so long to get to that!

@sunyab sunyab added pending push and removed blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed labels May 15, 2021
@pixar-oss pixar-oss merged commit d5d9576 into PixarAnimationStudios:dev May 17, 2021
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.

5 participants