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

Add support for building for Windows ARM64 devices #3430

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

anthony-linaro
Copy link

@anthony-linaro anthony-linaro commented Nov 19, 2024

Description of Change(s)

Add support for Windows ARM64 devices.

This was tested using the following command line:

python build_scripts/build_usd.py --no-python --onetbb --tests --no-imaging C:/WoA/OpenUSD/build_dir

Python is currently switched off due to lack of support for PySide on these devices. I am looking at that seperately.

I also had to disable imaging, as building boost appears to be broken when using newer CMake versions (similar issue to #3285)

TBB is set to OneTBB, as 2020u3 doesn't support these devices.

I also had to make a small modification in testWrapper.py, and change line 134 to pathPattern = pathPattern + r'(\S*)', as otherwise the escape sequence was invalid.

Checklist

[x] I have created this PR based on the dev branch

[x] I have followed the coding conventions

[ ] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

[x] I have verified that all unit tests pass with the proposed changes

[x] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10450

❗ Please make sure that a signed CLA has been submitted!

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@asluk asluk added the build Build-related issue/PR label Nov 20, 2024
@anthony-linaro
Copy link
Author

CLA sorted - approved by Tim Palmer

@pmolodo
Copy link
Contributor

pmolodo commented Nov 25, 2024

Any reason why the testWrapper.py change you mentioned isn't included here? Otherwise, lgtm

@anthony-linaro
Copy link
Author

I figured it was more related to the version of python I have installed (3.12.5), rather than the one used for validation on other devices - as I didn't have those devices/versions to hand, and knew the script already worked as-was on them, I opted not to change it

@@ -393,7 +393,10 @@ def RunCMake(context, force, extraArgs = None):

# Note - don't want to add -A (architecture flag) if generator is, ie, Ninja
if IsVisualStudio2019OrGreater() and "Visual Studio" in generator:
generator = generator + " -A x64"
if "ARM" in os.environ.get('PROCESSOR_IDENTIFIER'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the platform module be used here?
https://docs.python.org/3/library/platform.html#module-platform

Copy link
Author

@anthony-linaro anthony-linaro Dec 10, 2024

Choose a reason for hiding this comment

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

(repost from correct account)

It could be, but problems arise when running an emulated x64 version of python (which is easy to do by mistake, as that's what the website gives you by default).

This way checks the name of the processor directly, which is not affected by emulation.

@asluk asluk added the needs review Issue needing input/review by the repo maintainer (Pixar) label Dec 3, 2024
@asluk
Copy link
Collaborator

asluk commented Dec 19, 2024

"Python is currently switched off due to lack of support for PySide on these devices. I am looking at that seperately."

Hi @anthony-linaro , per our call yesterday, you also mentioned that the python bindings are failing to compile -- can you post the error messages you are seeing there?

"I also had to disable imaging, as building boost appears to be broken... "

@sunyab , is it expected that the command line

python build_scripts/build_usd.py --no-python --onetbb --tests C:/WoA/OpenUSD/build_dir

would attempt to build boost in v24.11+ ?

Thanks all!

@anthony-linaro
Copy link
Author

Hi @asluk - I have tried to reproduce the compilation errors, but am unable, so will chalk it down to an env issue - with things set up properly, I have most of the tests passing.

However, when I run ctest, I get this:

The following tests FAILED:
          2 - python_properties (Failed)
         47 - python_pickle1 (Failed)
         50 - python_pickle4 (Failed)
        225 - testPlug (Failed)
        666 - testUsdUtilsCreateNewUsdzPackageEditInPlace (Failed)
        708 - testUsdChecker1 (Failed)
        709 - testUsdChecker2 (Failed)
        710 - testUsdChecker3 (Failed)
        711 - testUsdChecker4 (Failed)
        712 - testUsdChecker5 (Failed)
        720 - testUsdChecker13 (Failed)
        778 - testUsdResolverExample (Failed)

I have attached the output file from the tests to this comment. Are these tests expected to fail? If not, do you have any idea why they might be? All env paths are set as instructed after build (bin and lib on path, python on pythonpath)

LastTest.log

@asluk
Copy link
Collaborator

asluk commented Jan 8, 2025

@anthony-linaro I don't think those test failures are expected -- can you try rebasing your branch to be on top of the "dev" branch? That is the most up to date commit. Thanks and happy new year!

@sunyab
Copy link
Contributor

sunyab commented Jan 8, 2025

@sunyab , is it expected that the command line

python build_scripts/build_usd.py --no-python --onetbb --tests C:/WoA/OpenUSD/build_dir

would attempt to build boost in v24.11+ ?

(sorry for the late reply, was out on vacation)

Yes, I would expect that to build boost in v24.11+. When imaging is enabled the --tests option requires the idiff tool from OpenImageIO for image diffs, and the version of OIIO the build script uses still requires boost.

@sunyab
Copy link
Contributor

sunyab commented Jan 8, 2025

However, when I run ctest, I get this:

The following tests FAILED:
          2 - python_properties (Failed)
         47 - python_pickle1 (Failed)
         50 - python_pickle4 (Failed)
        225 - testPlug (Failed)
        666 - testUsdUtilsCreateNewUsdzPackageEditInPlace (Failed)
        708 - testUsdChecker1 (Failed)
        709 - testUsdChecker2 (Failed)
        710 - testUsdChecker3 (Failed)
        711 - testUsdChecker4 (Failed)
        712 - testUsdChecker5 (Failed)
        720 - testUsdChecker13 (Failed)
        778 - testUsdResolverExample (Failed)

Hi @anthony-linaro, the python_* test failures are due to #3384, which is fixed in the dev branch. The other failures don't look familiar to me, so I'm not sure what's going on there.

@anthony-linaro
Copy link
Author

anthony-linaro commented Jan 8, 2025

Hi both, thanks for the responses!

@sunyab You're right, those python issues are now fixed, thanks to the suggestion from @asluk to merge dev.

Sadly, the list of errors has now changed, and I now get this:

The following tests FAILED:
        225 - testPlug (Failed)
        656 - testUsdUtilsCreateNewUsdzPackageEditInPlace (Failed)
        764 - testUsdChecker1 (Failed)
        765 - testUsdChecker2 (Failed)
        766 - testUsdChecker3 (Failed)
        767 - testUsdChecker4 (Failed)
        768 - testUsdChecker5 (Failed)
        776 - testUsdChecker13 (Failed)
        777 - testUsdChecker14 (Failed)
        778 - testUsdChecker15 (Failed)
        779 - testUsdChecker16 (Failed)
        780 - testUsdChecker17 (Failed)
        781 - testUsdChecker18 (Failed)
        788 - testUsdChecker26 (Failed)
        789 - testUsdChecker27 (Failed)
        799 - testUsdChecker37 (Failed)
        802 - testUsdResolverExample (Failed)

I have once again attached the logs: LastTest.log

Any suggestions of where I could start to look? It looks like it's not seeing the "Tf" python module. The path variable is correct, and added as specified when building (ie, modified PATH and PYTHONPATH) - is there another one that got missed somewhere? Was there an extra step required between build_usd.py and ctest for python to be able to see the module?

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -393,7 +393,10 @@ def RunCMake(context, force, extraArgs = None):

# Note - don't want to add -A (architecture flag) if generator is, ie, Ninja
if IsVisualStudio2019OrGreater() and "Visual Studio" in generator:
generator = generator + " -A x64"
if "ARM" in os.environ.get('PROCESSOR_IDENTIFIER'):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to follow the convention in apple_utils.py and extract this into GetHostArch https://github.com/PixarAnimationStudios/OpenUSD/blob/release/build_scripts/apple_utils.py#L66

@@ -393,7 +393,10 @@ def RunCMake(context, force, extraArgs = None):

# Note - don't want to add -A (architecture flag) if generator is, ie, Ninja
if IsVisualStudio2019OrGreater() and "Visual Studio" in generator:
generator = generator + " -A x64"
if "ARM" in os.environ.get('PROCESSOR_IDENTIFIER'):
generator = generator + " -A arm64"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with how its configured, but you might also need to support ARM64EC on Windows too ? https://learn.microsoft.com/en-us/windows/arm/arm64ec

@@ -35,15 +35,17 @@
#if defined(i386) || defined(__i386__) || defined(__x86_64__) || \
defined(_M_IX86) || defined(_M_X64)
#define ARCH_CPU_INTEL
#elif defined(__arm__) || defined(__aarch64__) || defined(_M_ARM)
#elif defined(__arm__) || defined(__aarch64__) || defined(_M_ARM) || \
defined(_M_ARM64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Along the lines of @dgovil's comment in build_usd.py, I think we'd also want to include defined(_M_ARM64EC) here and below?

@anthony-linaro
Copy link
Author

anthony-linaro commented Jan 8, 2025 via email

@dgovil
Copy link
Contributor

dgovil commented Jan 8, 2025

That's fair. At the very least, I'd suggest then adding a caveat to the README somewhere to document that EC isn't supported yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-related issue/PR needs review Issue needing input/review by the repo maintainer (Pixar)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants