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 a new IMPORTED target Vulkan::VulkanHppModule which contains vulkan.cppm #484

Merged
merged 5 commits into from
Jun 18, 2024

Conversation

sharadhr
Copy link
Contributor

@sharadhr sharadhr commented Jun 9, 2024

Added a new IMPORTED target and CMake library, Vulkan::VulkanHppModule that consumers can link against, to transparently use import vulkan_hpp;

@CLAassistant
Copy link

CLAassistant commented Jun 9, 2024

CLA assistant check
All committers have signed the CLA.

@sharadhr sharadhr changed the title Added a new IMPORTED target Vulkan::VulkanHppModule which contains vulkan.cppm Add a new IMPORTED target Vulkan::VulkanHppModule which contains vulkan.cppm Jun 10, 2024
Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

While I appreciate the improvements to Vulkan-Hpp that allow module usage, it cannot create errors for existing users by updating the CMake version unilaterally. 3.28 is extremely new and would punish anyone without the bleeding edge.

CMakeLists.txt Outdated Show resolved Hide resolved
@sharadhr sharadhr force-pushed the vulkan-cppm-cmake branch 2 times, most recently from 71e19f0 to 8a507df Compare June 10, 2024 18:23
Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

Changes look appropriate for what is being done - I would suggest squashing the commits so that the intermediate steps aren't in the git history. Same with the github PR description being out of date with the current set of changes.

@sharadhr sharadhr force-pushed the vulkan-cppm-cmake branch 6 times, most recently from 53aa415 to 4cc6a87 Compare June 14, 2024 23:03
@sharadhr sharadhr marked this pull request as ready for review June 14, 2024 23:07
@sharadhr
Copy link
Contributor Author

Squashed commits as requested. I've also made a few minor changes that I only noticed after the CI failures.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

You definitely addressed the issues I have with the original draft so thank you for doing that. Seems like there needs to be a special incantation to get the module to compile in the first place, but I’m unsure of what that would be as I haven’t tried getting modules to work with cmake myself.

@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 14, 2024

Seems like there needs to be a special incantation to get the module to compile in the first place

It's probably cmake --build path/to/build/dir to actually compile the module; however, built module interfaces (BMIs) are not portable and should be compiled by the consumer. Installing the interface file (vulkan.cppm) and having a consumer link to it should suffice.

It's strange that that particular CI step is failing here, but passes in my repository. With CMake 3.28, this should be entirely automatic with zero additional interaction by the user.

@oddhack
Copy link
Collaborator

oddhack commented Jun 15, 2024

Changes look appropriate for what is being done - I would suggest squashing the commits so that the intermediate steps aren't in the git history. Same with the github PR description being out of date with the current set of changes.

We can configure automatic squash-on-merge for this repo if you like.

@charles-lunarg
Copy link
Contributor

It's strange that that particular CI step is failing here, but passes in my repository. With CMake 3.28, this should be entirely automatic with zero additional interaction by the user.

Looking at your fork's CI run, it appears that it didn't run this branch, but only main. Hence it passed with flying colors since it wasn't trying to enable the c++ module.

GCC-11 and Clang-15 (apple clang) both are listed as not supporting the C++ Module Scanning that CMake requires, so I believe the best course of action is adding new CI jobs for newer compilers as well as disable the module support if the compiler isn't new enough in the other runs.

@charles-lunarg
Copy link
Contributor

Changes look appropriate for what is being done - I would suggest squashing the commits so that the intermediate steps aren't in the git history. Same with the github PR description being out of date with the current set of changes.

We can configure automatic squash-on-merge for this repo if you like.

I believe the current developer policy is rebasing which I prefer. Asking users to squish their commits if they so desire is suitable in my opinion.

@sharadhr
Copy link
Contributor Author

Looking at your fork's CI run, it appears that it didn't run this branch, but only main. Hence it passed with flying colors since it wasn't trying to enable the c++ module.

Ah, I just noticed that myself. I've modified the guard to test for the compiler and compiler version. I'm not quite sure how to modify the CI without dragging in huge from-source set-ups for latest Clang and GCC.

- Guarded behind `if()` version and compiler test
- Linked to `Vulkan-Headers` as a dependency
- Also added `CXX` to the `LANGUAGES` property of the project
@charles-lunarg
Copy link
Contributor

I just did some additional testing myself, and the CI fails because installing the headers on Windows-latest fails.
Gist is that its trying to install Vulkan-Module.lib which doesn't exist cause its not a regular library.
Doing some research about how to install modules, there may not be a consensus on how to do that just yet. Vulkan-Hpp doesn't appear to even try, it just installs vulkan.cppm as a header file and leaves it as that.

So I went ahead and branched on your change to test some stuff out. Adding an explicit install step revealed the underlying issue, and that is fixed by just not trying to install Vulkan-Module. Not sure if that is a solution, or just a workaround.

Regardless, while 2 of the 3 tests pass, the add_subdirectory test is still failing with an internal compiler error, which makes it quite difficult to know "what is wrong". https://github.com/charles-lunarg/Vulkan-Headers/actions/runs/9569931247/job/26383560260

@charles-lunarg
Copy link
Contributor

Good news is that the internal compiler error isn't due to running in the integration test. I realized the CI job needs to do an explicit build step now that the module may require explicit building.

https://github.com/charles-lunarg/Vulkan-Headers/actions/runs/9570194204/job/26384402318

Still an internal compiler error, but figured that should be said since it'll help figure it out.

@sharadhr
Copy link
Contributor Author

Ah, that ICE is known and reported, and should be fixed in VS 2022 17.11.

Installing a C++ module in CMake isn't something which is well defined currently.
Rather than add code which behaves poorly or flat out wrong, it is better to not
try to install a binary module for the time being. The vulkan.cppm file is still
included in the install so downstream users can still create a module from it.
This tests the Vulkan Module building without needing to run inside an integration
test. While theoretically the integration tests will exercise the build, the nature
of running the build inside of a test makes it more difficult to diagnose if the test
is poorly setup or if the build itself isn't working.
Adds the VULKAN_HEADERS_ENABLE_MODULE option to control whether to build the Vulkan-Hpp module.
This is necessary to allow CI to pass while waiting for the MSVC version 17.11, which fixes an
internal compiler bug, to be added to github actions runners.
@charles-lunarg
Copy link
Contributor

Okay that is GREAT news!

A known ICE with a "fix is coming" means I will go ahead and fixup the PR to do the requisite CMake changes, like adding a build option (which defaults to on, but can be set to OFF in case downstream users see issues like we did here). For the time being, windows has the module option set to OFF so that CI passes. Future work is to turn it on when MSVC 17.11 makes it into the runner.

Part of the changes is to remove the installing of the vulkan-module for the time being. It appears, after doing some research, that how to install C++ modules is still being worked on. It's prudent to air on the side of "don't" while the CMake devs work on documentation on exactly how to do it. At least in this PR the module is built & usable for users using add_subdirectory.

I also went ahead and added build & install steps to the CI - making it far easier to see where the build/install goes wrong.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

Approving again now that there is a large set of changes since the previous approval.

@charles-lunarg charles-lunarg merged commit e3c37e6 into KhronosGroup:main Jun 18, 2024
8 checks passed
@sharadhr
Copy link
Contributor Author

sharadhr commented Jun 18, 2024

Cheers; I appreciate the additional work done to get the CI happy! I'd eventually like to install the Vulkan-Module target too, since that was the point of this exercise; I'll try to discuss this with the CMake developers and see if I can bring something useful in a future PR.

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.

None yet

4 participants