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

[openxr-loader] Initial port #6339

Merged
merged 5 commits into from
Jul 15, 2019
Merged

[openxr-loader] Initial port #6339

merged 5 commits into from
Jul 15, 2019

Conversation

jherico
Copy link
Contributor

@jherico jherico commented May 8, 2019

A Khronos released SDK, the OpenXR loader is similar in function to the Vulkan loader, but currently is designed to be client built rather than installed like the Vulkan SDK. This PR adds support in vcpkg.

@NancyLi1013
Copy link
Contributor

Hi @jherico, thanks for your new port. I see the test results from the current CI system that the port failed on all triplets. Please refer to the following attachment.
failureLogs.zip

@jherico
Copy link
Contributor Author

jherico commented May 8, 2019

Whoops... looks like there's a build-time python3 dependency I didn't notice since I already have Python3 installed locally.

@NancyLi1013
Copy link
Contributor

@jherico, I checked the test results again from the current CI system after you added the python3 dependency commit. The port failed on all triplets. Please take a look at the failure log.
failureLogs_latest.zip

@jherico
Copy link
Contributor Author

jherico commented May 9, 2019

Thanks. Some of the errors seem to be originating from this error inside generator.py..

TypeError: argument should be string, bytes or integer, not WindowsPath

Investigation suggests that this is an error that only occurs on Python versions less than 3.6. I'll investigate to see what vcpkg fetches when a python build-time dependency is declared.

The linux build on the other hand is failing because of this:

Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)

I encountered this when I was doing my docker build, which is why I had to add it in the config, but on examining some of the other ports I'm guessing pkg-config isn't a part of the CI build environment for linux based triplets.

I'll investigate further for solutions. thanks for the feedback.

@Rastaban
Copy link
Contributor

Rastaban commented May 9, 2019

It looks like it uses Python 3.5.4 on windows see vcpkg_find_acquire_program. Maybe it is time to update to the latest version.

@Rastaban
Copy link
Contributor

Rastaban commented May 9, 2019

Updating Python3 in #6383

@jherico
Copy link
Contributor Author

jherico commented May 10, 2019

I tried applying this change locallly and the build still failed. The build system includes it's own python modules which need to be found by the python scripts and it's using the PYTHONPATH variable to ensure they're included.

However, the embedded python version used by vcpkg explicitly ignores PYTHONPATH in order to ensure total isolation from the host system. I'm looking into the python docs to figure out what I need to change in the OpenXR build process to make it work with embedded python.

@jherico jherico changed the title Add support for the OpenXR loader [openxr] Initial port May 12, 2019
@jherico jherico force-pushed the openxr branch 2 times, most recently from a5b0fab to 50b80ea Compare May 12, 2019 22:47
@jherico
Copy link
Contributor Author

jherico commented May 12, 2019

OK, I've added two patch files.

The first modifies the executed python scripts to explicitly add the required library paths to sys.paths rather than relying on the PYTHONPATH environment variable. That resolves the issue with using embedded.

The second modifies the presentation.cmake file used on Linux to hard-code the libraries needed rather than relying on pkg-config. This isn't actually the best approach. It appears the linkage information here is only used building the tests, which I've explicitly disabled for the vcpkg build. However, I think the proper fix here is to submit a PR upstream that makes the pkg-config usage conditional on the value of BUILD_TESTS. I've created that PR here and will retain this patch in the meantime.

@NancyLi1013
Copy link
Contributor

New test results from current CI system:

x64-windows master test notes
openxr Fail New Port
x64-windows-static master test notes
openxr Fail New Port
x64-osx master test notes
openxr Fail New Port
arm64-windows master test notes
openxr Fail New Port
x86-windows master test notes
openxr Fail New Port
x64-linux master test notes
openxr Pass New Port
x64-uwp master test notes
openxr Fail New Port
arm-uwp master test notes
openxr Fail New Port

failureLogs.zip

@jherico jherico force-pushed the openxr branch 2 times, most recently from d5efb06 to 1e4bc4a Compare May 16, 2019 17:43
@jherico
Copy link
Contributor Author

jherico commented May 16, 2019

The failures are still all caused by the embedded python being less than 3.6. However, I've added a patch file that should hopefully resolve this pending #6383

@brycehutchings
Copy link

The name "openxr" seems a bit too broad. Would "openxr-loader" be more appropriate?

@jherico
Copy link
Contributor Author

jherico commented Jun 6, 2019

@brycehutchings agreed, renamed.

@vicroms
Copy link
Member

vicroms commented Jun 24, 2019

/azp run

@vicroms vicroms changed the title [openxr] Initial port [openxr-loader] Initial port Jun 24, 2019
@vicroms
Copy link
Member

vicroms commented Jul 9, 2019

@jherico

The port fails to build on ARM and UWP, I suppose these failures were expected (am I right?) and I've added some explicit failure messages. If not, I'm OK with merging this PR as an initial version and patching support for the remaining platforms in a different pull request.

@jherico
Copy link
Contributor Author

jherico commented Jul 10, 2019 via email

@vicroms
Copy link
Member

vicroms commented Jul 15, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vicroms
Copy link
Member

vicroms commented Jul 15, 2019

Thanks for the PR!

@vicroms vicroms merged commit 0c8139d into microsoft:master Jul 15, 2019
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