-
Notifications
You must be signed in to change notification settings - Fork 591
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
Fix or disable clang format checks #937
Comments
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. |
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 |
Fixed via #939. |
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. |
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. |
It looks like either the output of clang-format ( 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 |
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. |
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 |
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. |
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 I agree something that just reformats it on the way in or provides an actionable commit is preferable. |
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 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" . |
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. 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. |
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
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. |
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. |
As part of our CI we run clang-format to ensure that code is properly formatted. Sadly IDEs use different clang-format versions. Visual Stuido 2022 e.g. uses an newer version than we do. For people using MSVC 2022 (like me), this makes it pretty much impossible to write code that doesn't fail CI. We somehow need to do something about this urgently.
The text was updated successfully, but these errors were encountered: