Fix Vulkan build issues, enable Vulkan build in CI #6709
Conversation
Goober5000
added
the
vulkan
|
@vmedea In order to fix the CI checks, could you change to |
Done! |
|
D'oh...
@vmedea Can you try it with brackets and quotes? So: |
|
Did that now! But it looks like the Linux build image doesn't have Vulkan installed, or the path needs to be passed in: |
|
Well I'm not exactly sure what to do here, but try taking the following code (copied from the Windows and Mac sections) and putting it in the Linux section: |
vmedea
force-pushed
the
fix-vulkan-build
branch
3 times, most recently
from
732ba0d to
475548b
Compare
|
It looks like the problem it now runs into comes from: https://github.com/scp-fs2open/docker-images/blob/main/build/linux/Dockerfile#L14 The path Edit: But that script already installs Aargh. Installing Not sure if this is a mistake in the package or something else. Ubuntu 24.04's package has them, but that's the distro's default package not LunarG's. It's intentional: |
|
Huh, good detective work. 👍 I've approved and merged the other ticket. |
|
Well merging that ticket produced an error in the build-images repository: |
My guess is that something in kitware's cmake repository changed since last image build. None of the vulkan packages depends on any version of This fixes the conflict locally: scp-fs2open/docker-images#5 |
|
Great catch! The PR has been merged so I restarted the workflow checks. EDIT: Bleh, it's back to failing on the |
|
Bumped the image, it gets past the cmake stuff now ! 🎉 It's still failing, but at least in a useful way, will look into handling the return codes here: |
|
No failing checks anymore, but it looks like 6 of the Linux builds are hanging (have been since yesterday). There appears to be no way to view the logs in progress. |
|
Strange, it's attempting to check more builds than it has run. Those checks don't even have logs; they are waiting for something that doesn't even exist. I have to assume this is related to the change in build configuration, but I'm not sure how. |
|
Ok let me double check a few things. Thanks for your perseverance! |
|
I'd like to ask if you could remove your workaround in |
Sure, restored the yaml change instead. |
|
Thanks. I think I'm getting closer to solving this. Could you revert the change from this comment above, so I can see what that does? |
|
In the current PR, at least, it looks like |
Sorry, thought reverting to before making it a yaml list. It's gone completely now. Doesn't look like it's building with Vulkan now: There's some clang tidy error though, will look into it: Edit: fixed this by passing the right define to |
vmedea
force-pushed
the
fix-vulkan-build
branch
4 times, most recently
from
8b53f2d to
028ab50
Compare
|
I believe you've squashed the last issue. Well done! All the checks have passed, there are no more fiddly cmake configuration changes, and that extra clang-tidy arg looks like it should work with or without Vulkan. I want to double-check with someone before I approve it, but I think this is good to go. Thanks again for your perseverance! |
|
Yess! The only thing is, as far as i can see vulkan is no longer enabled for the CI. The vulkan code is checked by clang-tidy, which is good, but not compiled in any of the runs: "Enabling Vulkan support" is misleading here. It comes from OpenXR, not from FSO's own cmake file. |
|
Good point. I'll see about adding it to the CI runs. |
|
@vmedea So here is a conundrum. Take a look at this line: If Vulkan is present, that flag should be automatically included. So it shouldn't need to be explicitly specified to clang-tidy. ...Actually, that probably answers the question. The build is being performed without Vulkan support, but clang is forced to run on that file because the file was edited as part of the PR. So let me ask you to try the following:
(I believe, if all of that works, we'll have to override, suppress, or force-approve the clang-tidy failures since clang-tidy is created a build configuration that shouldn't normally be possible.) |
|
@vmedea bump |
When i checked it seemed to me like there is no mechanism that passes the cmake-provided flags to clang-tidy. But it'd make sense. E.g. there's nothing here that passes any compiler flags at all: https://github.com/scp-fs2open/fs2open.github.com/blob/master/ci/linux/clang_tidy.sh#L22 . It's not launched through cmake, nor make/ninja where
Will try! |
In 133b033, SCP_vector was made into a subclass, so direct assignment from the vulkan-hpp result isn't possible anymore. Use an intermediate vector.
Because these functions can return statuses that aren't strictly an error (and thus don't raise an exception), but might have to be handled, they return a `vk::Result` that's `[[nodiscard]]`. Ideally some of the result statuses would be handled here, but to get it to compile, cast the result to void.
Switch to the new Linux docker image with Vulkan headers from scp-fs2open/docker-images#7, for all github builds.
Ah, I see what you mean. I believe what's happening is that the cmake configuration causes that define to be associated with the So please put that definition back into the script and let's see what happens. In fact, to be consistent with the cmake file, put all the definitions in: |
`clang-tidy` doesn't pass in the compilation flags normally provided by cmake, so add the vulkan flags manually to make sure that everything is checked as it should be, and thus prevent it from choosing the static dispatcher and complaining about `.init()` being used.
Exactly. In some projects, a header file is generated with options and defines, and included. But not here, it's all passed on the compiler command line by the build system. Which is fine, but it means external tooling can't access them.
Yes good idea, added! |
In 133b033,
SCP_vectorwas made into a subclass, so direct assignment from the vulkan-hpp result isn't possible anymore. Use an intermediate vector.