KhronosGroup / Vulkan-Headers

Vulkan header files and API registry
https://www.vulkan.org/
Other
846 stars 220 forks source link

Add a new `IMPORTED` target `Vulkan::VulkanHppModule` which contains `vulkan.cppm` #484

Closed sharadhr closed 5 months ago

sharadhr commented 5 months ago

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

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

sharadhr commented 5 months ago

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

sharadhr commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

charles-lunarg commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

charles-lunarg commented 5 months ago

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.

sharadhr commented 5 months ago

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.