Fix or disable clang format checks about vulkan-samples HOT 15 OPEN

Personally, I'd favor pre-commits as well. However, we have to think of maintainers as customers/end users. We can't mandate that everyone have a particular tool or setup if we want to maximize the number of people that can contribute to the project. So pre-commits are great for us on platforms that are setup for it, but less great for people who either have never used it or run into issues with it. Meeting them in the middle with a path to get out of hours of trying to get past CI seems like a good compromise.
What I did was create a very quick PR which allows the CI to save the artifact that is generated via the clang-format job. That way if you make a PR and it fails, you can download the diff and hopefully that will make it easier to work with. Might have to reformat the diff so it's easier to consume, or just save off the new files and let those be downloaded? But goal is to meet people in the middle so when they have a build issue we're at least doing a small step to give them a quick path out of the issue. Just download the artifact run patch and continue on.

from vulkan-samples.

You can explicitly tell MSVC which clang-format binary to use.

from vulkan-samples.

I'm aware of that option. But tbh. I'd prefer if we had a way where no changes to a IDE setup would be required. I don't want to adjust a global setting just for a single project.

from vulkan-samples.

Does #939 help resolve this for you?

Out of the options we tried the only way we can fix this for MSVC out of the box is to update the clang format version in the CI. Which in turn will just break again in the futur when MSVC updates its dependencies

With #939 all users should have the same experience after installing pre-commit. It also will allow us to automate the copyright checks in the future too

from vulkan-samples.

Fixed via #939.

from vulkan-samples.

Reopening again. I can't get one of my PRs to pass clang-format checks. Been trying for hours. See https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/8381317875/job/22952552513?pr=887

The CI output is utterly useless. It's a gigantic one liner that tells you roughly where the problem is, but not not WHAT it actually is.

It's both infuriating and frustrating.

Can we add some way to forcefully skip this check?

I'm probably not smart enough, but for most of my PRs I spent more time fixing CI related things than actually working on the code.

from vulkan-samples.

It's getting worse. So I downloaded LLVM and told VS to use clang-format from that instead (as in Bradley's screenshot). It did format a few things differently, and now I get even more clang format errors in the CI.

from vulkan-samples.

https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/8381317875/workflow?pr=887
The CI output is utterly useless. It's a gigantic one liner that tells you roughly where the problem is, but not not WHAT it actually is.

It looks like either the output of clang-format (steps.clang-diff.outputs.changes) is being collected up word-wise into the variable or is being evaluated word-wise and thus collapsing whitespace.

Someone with good enough YAML-fu could take a look to see how this could be changed to output the diff verbatim. My YAML ability is non-existent and a quick search didn't reveal anything. Last people to touch that workflow file at that step are @charles-lunarg and @TomAtkinsonArm

from vulkan-samples.

I've been thinking about this. Maybe a solution is instead of having the CI notice differences to have the CI save the changes to a new file and commit it back while failing the build, so all you have to do is sanity check that file, move it over to the offending one and then you'll pass CI?

That way at least there's a quick path towards solving the under-laying issue that clang-format isn't meant to have any backwards compatibility. The only real way is to have one clang-format version that the whole project uses; which doesn't bode well when people update their IDEs (something we want to encourage). The only other solution is to not enforce clang-format.

from vulkan-samples.

Another alternative is just to add the nicely formatted log of the change file in the CI. In the worst case scenario you will be able to read and manually fix the discrepancies.

I ran pre-commit manually on Saschas PR and it seems to have fixed the files. I dropped a comment on that PR with an example command to run

The final alternative would be to bump our clang tooling to the latest supported by MSVC. Fairly easy to do. It may remove some barrier to entry

I do think that pre-commit is the most sensible solution as it pulls the correct versions and is IDE agnostic. But I am bias, I use this at work across multiple repositories and we practically automate all tests, lints, formats etc with it across multiple tech stacks (Web, native c++, unreal engine, go, I think there's some Java in there too but I stay clear from those repos)

Let me know if we want a version bump and I'll sort out the docker images

from vulkan-samples.

We have clang-format in https://github.com/LunarG/gfxreconstruct CI GH Actions and I regularly cut-and-paste the diff reported in our CI (e.g. https://github.com/LunarG/gfxreconstruct/actions/runs/8354729132/job/22868539456) as a patch to fix clang-format on my machine. It's lazy, but in the absence of a readily-reproducible clang-format that works everywhere it does the job. I like #996, and even better because it's a downloadable artifact.

I agree something that just reformats it on the way in or provides an actionable commit is preferable.

from vulkan-samples.

If the problem is that some people have a different version of Clang on their system, wouldn't it be simpler to just include an additional script (or an additional command line option for the existing script) that will use a docker container to execute the correct clang-format version against the files?

I don't think it's an unreasonable ask to say "You have to have either LLVM version 15 or Docker as part of local setup to develop for this repo" .

from vulkan-samples.

It's not unreasonable to say that version X of clang-format is in use on the developer machine. Nor is it unreasonable to say that we have a dockerfile which they could install/use. Nor is it unreasonable to say that there's pre-commit scripting support.
However, in the unlikely event those don't get used, it's not a bad thing for us to provide a path out of having this issue. I personally don't use either pre-commit nor the dockerfile as I primarily do my dev in CLion, which supports using pre-commit, or any version of clang-format directly. However, every now and again even I run into problems; not that I claim to be a competent programmer; I think it's a small thing to offer another path towards getting people to work with our repo.

Also, something else to bare in mind. This project is meant to be able to work on platforms that might not even have support for LLVM.

from vulkan-samples.

I don't mean to suggest that we shouldn't do any particular improvement that's been suggested. Simply that providing the option of using a container based clang-format might be an easy to accomplish way to solve the original problem.

This project is meant to be able to work on platforms that might not even have support for LLVM.

Do you mean "work" in the sense of "people can build and run the examples" or in the sense of "people are actively writing code and submitting PRs"? Because I assume that the issue of LLVM availability is really only a concern to the latter. Most of my experience developing on platforms with sparse options for tooling have been embedded stuff that ultimately requires some kind of host system to work with, but I haven't spent a lot of time working on GPU stuff beyond the desktop environment.

from vulkan-samples.

The way I think of it is, if Vulkan can work on a given platform, then it'd be nice if samples could work on that same platform.

from vulkan-samples.