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] Update to latest version, official repo #12060

Merged
merged 12 commits into from
Aug 20, 2020

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Jun 22, 2020

Describe the pull request

  • What does your PR fix?

Updates the OpenXR loader to the most recent released version, 1.0.11

Additionally, the loader was previously being built from a fork to include the openxr.hpp header. The hpp header is an official OpenXR Khronos project, but it's not currently integrated into the build of the OpenXR SDK itself. In order to work around this I previously used a fork where I manually included the header, but Vcpkg practices suggest it's preferable to make such modifications via patch or change to the portfile.

This this PR switches the port to use the official Khronos repository for the OpenXR SDK. Additionally at build time it also clones the required repositories to build the openxr.hpp file, and also includes a small patch file to correct a bug in the generated header.

The onus is therefore on the port maintainer to ensure that when they update the version of the OpenXR loader, they also update the refs of the additional repositories to match. The OpenXR-SDK-Source repo should have the same version tags as the main OpenXR-SDK repo, but the OpenXR-hpp repo doesn't use version tags and should just use the current HEAD. The port maintainer should also ensure that the patch file is updated as necessary or removed once the bug is fixed in the upstream repository.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

ARM and UWP based triplets are not supported. This represents no change from the existing version.

Yeah, I think

@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Jun 23, 2020
ports/openxr-loader/portfile.cmake Show resolved Hide resolved
ports/openxr-loader/portfile.cmake Outdated Show resolved Hide resolved
ports/openxr-loader/portfile.cmake Show resolved Hide resolved
ports/openxr-loader/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@jherico
Thanks for this PR,
Have you tested the feature vulkan and if it can work well on all platforms?

@jherico jherico requested a review from NancyLi1013 June 23, 2020 15:50
@NancyLi1013
Copy link
Contributor

Have you tested the feature vulkan ? Does it can work on x86-windows, x64-windows and x64-windows-static triplets?

@jherico
Copy link
Contributor Author

jherico commented Jun 24, 2020

My understanding was that the Vulkan port only checked for the presence of the Vulkan SDK. I've tested it with the x64-windows and x86- windows triplets, but not x64-windows-static.

@NancyLi1013
Copy link
Contributor

Yes, your understanding is right. Thanks for your update.
Can it work on x64-window-static ? There is nothing need to be done for this PR if you can make sure that it can build with feature vulkan on these triplets.

@jherico
Copy link
Contributor Author

jherico commented Jun 25, 2020

I've checked on x64-windows-static via ./vcpkg.exe install --triplet x64-windows-static openxr-loader[vulkan] It appears to build just fine.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Jun 29, 2020
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 removed the info:reviewed Pull Request changes follow basic guidelines label Jul 27, 2020
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Aug 5, 2020
ports/openxr-loader/portfile.cmake Outdated Show resolved Hide resolved
@strega-nil strega-nil added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Aug 12, 2020
@NancyLi1013
Copy link
Contributor

@jherico
Could you please fix the issue on x64-linux?

The /lib/cmake folder should be merged with /debug/lib/cmake and moved to /share/openxr-loader/cmake.
Please use the helper function `vcpkg_fixup_cmake_targets()`
The following cmake files were found outside /share/openxr-loader. Please place cmake files in /share/openxr-loader.

    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/lib/cmake/openxr/OpenXRConfigVersion.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/lib/cmake/openxr/OpenXRConfig.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/lib/cmake/openxr/OpenXRTargets.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/lib/cmake/openxr/OpenXRTargets-release.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/debug/lib/cmake/openxr/OpenXRConfigVersion.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/debug/lib/cmake/openxr/OpenXRConfig.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/debug/lib/cmake/openxr/OpenXRTargets.cmake
    /mnt/vcpkg-ci/packages/openxr-loader_x64-linux/debug/lib/cmake/openxr/OpenXRTargets-debug.cmake

The /debug/lib/cmake folder should be merged with /lib/cmake into /share/openxr-loader
Found 3 error(s). Please correct the portfile:
    /agent/_work/1/s/ports/openxr-loader/portfile.cmake

ports/openxr-loader/portfile.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

This looks great :)

ports/openxr-loader/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Aug 19, 2020
@NancyLi1013
Copy link
Contributor

LGTM now. Thanks for your hard work. @jherico

@BillyONeal BillyONeal merged commit 970fdf4 into microsoft:master Aug 20, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

remz1337 pushed a commit to remz1337/vcpkg that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants