Based on our recent discussion, I have created a ruleset to add some new requirements for committing to the main branch. Technically, they are more like recommendations than requirements, because anyone with commit access can bypass these requirements if they want:
Must use a pull request.
Pull Requests must have at least one approver.
The code_formatter and buildkite CI checks must pass.
When you push directly now, you will get a warning that says you have bypassed the repository rulesets. On pull requests, you will need to check an extra box in order to merge the PR if not all these requirements are met.
code formatter could have fails when changing historical code. I think our generic rule is not to bulk reformat but rather follow the existing code style
buildkite has spurious fails and there is no way to restart it manually
I think the intention is that you not have to do anything different than what you are already doing, so I would just bypass the rule and commit in that case.
We have long standing practice on it being acceptable to land e.g. small NFCs directly.
code_formatter is complete noise. It generates spurious failures on surrounding code. I am actively ignoring it at this point.
buildkite cycle times after a rebase and push is longer than I’m willing to wait on an approved review. I frankly find buildkite a pretty poor signal for the actual buildability of the patch. I regularly have both commits which build fine with applied to tot flagged as broken, and worse, changes which don’t build against ToT reported as fine due to stale base revisions.
Note that you can still do this, there is just a warning printing on the output when you push.
Admittedly the phrasing is a bit strong here, and quite unfortunate considering the intent. Looking at the discussion, this wasn’t something we wanted, but we can’t have the warning on pull-request missing approval without also requiring pull-request apparently.
Isn’t it just git clang-format ? It should be limited to the code you’re touching or it is a bug I think. (I always run git clang-format locally anyway)
I just looked up the data and the code_formatter success rate is 891 passes in the last 1020 runs ~ 87%. It would be good if we could get tickets filed when it gives spurious failures.
One place where I’ve seen the formatter fail and have ignored it is when touching a large enum (for example in in Triple.h). I add a value and now clang-format wants to reformat the whole enum. Is that considered a bug? If I were a clang-format developer I’d probably argue that it’s not, and in most cases that’s the behavior I’d want as user.
Are there enough cases where people are ignoring the formatter that it’s worth making it mandatory? I’d consider it part of the review process to call this out if authors don’t follow up. When I see something I don’t think is reasonable (like the example I gave above) I leave a comment explaining why I’m ignoring the formatting job.
+1, I generally tend to wait for all the jobs to complete but sometimes the Windows builder takes so long that I don’t know if it’s really still queued or something timed out.
Not a bug in clang-format, IMO. Arguably a bug that Triple.h (or whatever) isn’t compliant to start with.
Getting the project as a whole to be more compliant in the first place would probably reduce some of these “spurious” reports. Seeing a number like 87% passing on the pre-check doesn’t tell us whether clang-format has a problem or the file you touched hasn’t been brought up to spec.
And what if I sent a PR, but no one is reviewing it. Also I am the maintainer of the area and I think we need to land it in weeks if no more comments come in.
In this case, we can’t click the merge button since there is no approver. Should we close the PR and commit it separately?
We, as a project, have had the discussion before about whether we wanted to allow bulk format changes. We decided not to.
We, as a project, have had discussions about whether unrelated formatting changes are allowed in commits. Our long standing answer is no.
Our guidance from the developer policy reads: "Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to. Also, try to separate formatting or whitespace changes from functional changes, either by correcting the format first (ideally) or afterward. "
If the tooling that we had added to github is incompatible with our documented and long standing policy, we should fix the tool.
To me, an 87% pass rate is pretty dismal on the surface. Do you happen to have access to that stat specifically for landed changes? If our contributors are choosing to ignore the input of the tool at anything close to that, that would seem a pretty major argument in favor of disabling the tool entirely, much less not enabling it as mandatory. If on the other hand, that’s the initial failure rate and submit rate is much higher, that would tell a different story.
@tstellar I would like to specifically request that you revert this change while the discussion proceeds.
Enabling this without prior discussion and consensus seems to violate our community standards. Leaving it enabled after objection would seem to violate our norms around reverts of changes with unexpected impact. Let’s get back to a working state so that we can discuss (review) the proposed changes.
(Edit - Leaving the wording above so folks see what I said, but I realized the first part is too strong. There was prior discussion here, it just hadn’t obviously come to a consensus with a clear publication of intent.)
For the record, here’s what a push to main currently shows:
remote: Bypassed rule violations for refs/heads/main:
remote:
remote: - Changes must be made through a pull request.
remote:
remote: - 2 of 2 required status checks are expected.
remote:
To github.com:llvm/llvm-project.git
1e9924c1f248..b75cad44136e main -> main
We have conflicting policies here. One policy is that code should be clang-format clean. Another policy is not to proactively make things conform to the first policy.
Rampant changes in formatting when the formatting policy was first introduced seemed like a not unreasonable policy. But that was a long time ago, and a whole lot of code has become conformant since then. I don’t know what the actual conformant rate is (the 87% number is a hint, but not a proper measurement of the project as a whole) but if we’re getting moderately close, then going ahead and finishing the job seems entirely reasonable. Preserving a policy to not conform to another policy just seems foolish at that point.
I know we have had these discussions before, but IMO that was then, this is now. The environment has changed and it is appropriate to reopen the discussion.
To be clear here, I’m entirely open to having a discussion about changing our policy. I’m even moderately of the personal opinion that we made the wrong decision as a project when we made it. However, it is a significant policy change and deserves to be treated as such from a discussion and consensus perspective.
If someone cares enough to propose a new policy for code formatting and go through all the consensus building required, feel free.
Me too. We’ve had two or three style revisions in the dozen years I’ve worked with LLVM, and each time said no bulk changes. Now we have a code base with multiple different styles. It drives me nuts sometimes; the benefit of a single consistent style was one of the very first lessons I learned as a professional coder.
I should have some free time next week to collect data as a basis for doing that RFC. I’ve been making noise in this direction for a while, it’s past time to act on it.
I feel the same way. With Python and Black we decided to go the other route (bulk reformat everything) and personally I consider it a big success. I’d be supportive of doing the same thing here, motivated by the check.
