On Wed, Aug 14, 2024 at 4:49 PM Christopher <ctubb...@apache.org> wrote:
> On Wed, Aug 14, 2024 at 6:06 PM Yuxuan Wang > <yuxuan.w...@reddit.com.invalid> wrote: > > > > >by simply running `gradle spotlessApply`. It was trivially easy to fix, > > and anybody could have done it. > > > > but that requires "anybody" to have gradle setup on their dev machine, > and > > not everybody is java/jvm developer with gradle setup :) > > > > yes for people contributing java/kotlin PR that breaks build they > *probably* > > already have gradle setup, but there are still exceptions (like me trying > > to fix the failed build just by looking at the code without gradle > setup). > > that's not always true *after* the broken code is already merged and > > someone just tries to fix the build. > > > > and I think I've seen it at least a few times that the github dependency > > bot tries to upgrade the spotless plugin to a new version which would > start > > to fail the code that passed before for style reasons. so people need to > > either reject the PR, or have gradle setup and run gradle to fix the code > > that's starting to break before merging the dependency PR, which I think > > essentially reduces the pool of maintainers to response to those > dependency > > PRs (while our number is already not great). > > > > I think it's reasonable for a java contributor to have it set up > already, but I understand it can be a problem for others. So, I agree > that committers need to do a better job of rejecting things that fail > the checks so the responsibility isn't left to others to fix it > afterwards. The same is true for dependabot PRs. Those should not be > merged if they cause a failure. > > > what do you think if we keep the spotless and ktfmt steps, but add > > `continue-on-error: true` ( > > > https://protect.checkpoint.com/v2/___https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions%23jobsjob_idstepscontinue-on-error)?___.YzJ1OnJlZGRpdDpjOmc6OWI0ZTQ3YmI0NzZlOTI3OWQ4ZGYzNzY2OTBmODRhNzg6NjoyNzVkOjZlNTZiNjVmNWNkNDA1MDVlY2Q4MTEwYWYwNDljZGZlOTIxNmFiYjliOTMzODAyOTMwMjQ1YTAxNzdhNzM1MWQ6cDpUOk4 > > if the code truly not builds (like the recent commit) it would still fail > > the later steps on the CI, but if it's purely style issue, that at least > > allows us to run cross-tests? > > I think that's a good solution, but I worry that it will encourage > committers to ignore the checks and accept PRs that break checks. > yea I have the same concern. I think currently the build is fixed so I'm good. maybe we can bring this up again the next time it fails :) > > > > > On Wed, Aug 14, 2024 at 2:37 PM Christopher <ctubb...@apache.org> wrote: > > > > > I strongly disagree with this. spotless doesn't just standardize > > > formatting, but also checks for a wide variety of code quality issues > > > that people should address. Just because the check is getting ignored > > > and not fixed right away doesn't mean the solution is to remove the > > > check. It means more attention needs to be paid to fix what the check > > > is failing. > > > > > > Also, spotless depends on compilation step. It's just the entry point > > > into the build, not just a formatting check. If you remove the > > > spotless check without adding a compilation step, then no checks are > > > done at all that verify the code even builds. > > > > > > Also, it's very easy for anybody to fix most spotless checks. > > > > > > The real problem is that people are accepting PRs and merging them > > > when the PR fails the check. The person who made the edit that caused > > > the breakage should fix their PR before accepting it. This isn't just > > > a problem for formatting/style issues. This is a problem with basic > > > compilability. Recently a commit got into the Java code that didn't > > > even compile at all, and ended up needing a new PR to fix it. If the > > > build failure were attended to instead of ignored, that wouldn't have > > > happened. > > > > > > I trivially fixed the spotless check this afternoon in > > > > > > > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3019___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6NjpjZDdjOmQ5N2ZhNzBlNDdiZjRiNWVhOWUyYjRkMDRlMWJhOWQ1YzM5NTRiYzBjZTJjYWJkY2IxODZhZGViYjNmMDhmNGU6cDpUOk4 > > > by simply running `gradle > > > spotlessApply`. It was trivially easy to fix, and anybody could have > > > done it. > > > > > > I'm a strong "no" against removing the spotless check. > > > > > > On Wed, Aug 14, 2024 at 4:53 PM Yuxuan Wang > > > <yuxuan.w...@reddit.com.invalid> wrote: > > > > > > > > I created > > > > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3020___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6Njo4MmZkOjQ2MjYxNWYyYjdiN2NlYWRhMjgzOGMxNTUxMThkZTFkMTg3ODBkMGI3MzQ1ZmE4MGUwOTMyOTY0ZjYyZGExZTI6cDpUOk4 > > > as a test to see how > > > > that works. > > > > > > > > That PR also includes changes in > > > > https://protect.checkpoint.com/v2/___https://github.com/apache/thrift/pull/3019___.YzJ1OnJlZGRpdDpjOmc6NWYxODdjYjFkYmUzM2IzNjI4NmRhNDdiM2ZhMWZmYWU6NjpjODhkOjE3MWQ2ZTc3YTQyZDlhZmQzMTE3ZDc5ZWFjNDI5NzIwN2U5MjczYmIyOGE5Nzc5MzI0MDNmZGY4ZjJjMGNkNTk6cDpUOk4 > > > , > > > > as otherwise the java lib code won't even build. Which I think shows > > > > another issue with the spotless check currently: it hides bigger > problem > > > > that could cause the java lib to stop building, as when spotless > check > > > > fails we stops CI early and won't know whether the java lib builds. > > > > > > > > On Wed, Aug 14, 2024 at 9:42 AM Yuxuan Wang <yuxuan.w...@reddit.com> > > > wrote: > > > > > > > > > From my observation of the past few years, every time the github > > > > > dependency bot upgrades the spotless plugin version, it will start > to > > > fail > > > > > something that passed before. And it usually takes months for > someone > > > to > > > > > fix those, if they are ever fixed. > > > > > > > > > > While having some enforcement on styling is great, making those > > > > > enforcements to break builds (and also block cross tests from > running) > > > for > > > > > months is not. If we don't have the resources to keep the styles > > > > > up-to-date, then maybe sacrificing some styling in favor of actual > > > cross > > > > > tests is a better approach? > > > > > > > > >
