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

[arch, plug] fix paths that contain unicode char for plugInfo.json #1987

Merged

Conversation

nvidia-jomiller
Copy link
Contributor

Description of Change(s)

arch: ArchGetAddressInfo was using GetModuleFileName instead of GetModuleFileNameW on Windows. This prevented the objectPath to be correctly returned should the module found at the address containing a unicode character.

plug: _ReadPlugInfoObject did not properly handle the case of a UTF-8 encoded string on Windows. This would result in the plugInfo.json file not being loaded if it resided at a path containing a unicode character

Fixes Issue(s)

  • I have submitted a signed Contributor License Agreement

@nvidia-jomiller
Copy link
Contributor Author

It looks like the build for Windows on Azure is hanging. It's working correctly on Windows 10 but will investigate when time permits.

The line where this seems to be hanging is here:
https://github.com/PixarAnimationStudios/USD/blob/release/build_scripts/build_usd.py#L266

@sunyab
Copy link
Contributor

sunyab commented Aug 17, 2022

Filed as internal issue #USD-7554

@nvidia-jomiller nvidia-jomiller force-pushed the dev_unicode_path_fix branch 2 times, most recently from 00634cc to a7513e6 Compare August 18, 2022 14:51
arch: ArchGetAddressInfo was using GetModuleFileName instead of GetModuleFileNameW on Windows. This prevented the objectPath to be correctly returned should the module found at the address contained a unicode character.

plug: _ReadPlugInfoObject did not properly handle the case of a UTF-8 encoded string on Windows. This would result in the plugInfo.json file not being loaded if it resided at a path containing a unicode character
# To keep things consistent we'll force UTF-8 for non-interactive
# streams (which is recommended by Microsoft). See:
# https://docs.python.org/3.6/library/sys.html#sys.stdout
# https://docs.microsoft.com/en-us/windows/win32/intl/code-page-identifiers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to track down the build issue with Windows Azure and it has something to do with what default encoding is set for the terminal running build_usd.py. I made a small change to GetLocale() in build_usd.py to force UTF-8 on non-interactive terminals for Windows.

Specifically sys.stdout.encoding on Windows will use the ANSI codepage for non-interactive streams and Microsoft recommends to use UTF-8 or UTF-16 over ANSI codepages

Copy link
Member

Choose a reason for hiding this comment

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

nice find

# for Windows
pxr_create_test_module(TestPlugModuleUnicode
INSTALL_PREFIX PlugPlugins
DEST_DIR TestPlugMödüleÜnicöde
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's worth pointing out that DEST_DIR will be a created directory which contains unicode characters to verify the fix

@nvidia-jomiller
Copy link
Contributor Author

I know this isn't a very high priority bug, but it would be great if this could get reviewed and merged into dev. At least a few users have ran into this same problem and one has verified that it fixes the issue:
https://groups.google.com/g/usd-interest/c/Lit5oKhejoE/m/2aKR_8h-BQAJ

Thanks!

@spiffmon
Copy link
Member

We're scheduling this for the next release, @nvidia-jomiller - thanks!

@meshula
Copy link
Member

meshula commented Mar 5, 2023

@nvidia-jomiller I was wondering if you've tried this under Windows 11? Although the fix seems to address the specific case in the issue title, I haven't been able to get "unicode user name in path" case mentioned in the usd-interest message working without further work in addition to the changes you propose here, and I'm wondering if it's a Windows 10 vs 11 thing.

@nvidia-jomiller
Copy link
Contributor Author

@meshula I haven't tried under Windows 11 as my Windows machine is still on Windows 10. I assume you're testing on Windows 11? What additional work did you need to do with the usd-interest case to get it working?

@meshula
Copy link
Member

meshula commented Mar 6, 2023

TfGlob failed to discover the pluginfo.json files and I made a fix that I'm not particularly excited about. If you are interested in seeing some of the issues around unicode in filenames versus toolchains (not USD specifically), I have been collecting findings here... https://github.com/meshula/m-n

@pixar-oss pixar-oss merged commit a7f67e3 into PixarAnimationStudios:dev Aug 29, 2023
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