On Sun, Mar 8, 2020 at 2:50 AM Alan Carvalho de Assis <acas...@gmail.com> wrote: > > Hi Xiang, > > Normally Greg, Abdelatif, I and others are fixing these nxstyles > issues when someone submit a patch. It is described in the legacy > process of merging PR: > https://acassis.wordpress.com/2020/01/02/the-old-way-nuttx-workflow/ >
First, I would suggest that all commiters stop to modify the code in PR branch and then merge into master branch. This process most likely generate build break once per week. > I don't know if it is fair or unfair to share/transfer this > responsibility to people submitting patches. But I recall how it was > boring to submit patches to Linux kernel, they used to reject my > patches many times until all issues were fixed. > Yes, I also submit the patch to Linux kernel, but kernel maintainer just request the people fix the issue in his patch, not the problem already exists in the code base, right? > The reported errors from nxstyle could be included directly as a > comment in the PR, then the author could see it and fix it. > > If we relax the process I think more styles violations will be > included on mainline. In the order hand if people start to care about > it eventually all files will get rid of issues. > As I reply to David, we don't relax the changed made by people, all lines in the patch still go through nxstyle check, what I suggest is that we ignore the unmodified part. The codebase never become the worse with this relaxation. The maintainer still can provide the followup PR to fix the style issue in the rest files to make the mainline better. > More important the release a new version ASAP is to guarantee we are > building a solid base to make the entire process more robust. I think > we are going in this direction. > Yes, this is the key point why I am asking this question: we need stable the mainline and make the first apache release. But how we can get the stable mainline? The minimum requirement is that all config/host pass the build. The precheck is online on now which is only method to guarantee that each new patch don't break the build, but most commiters just ignore the error reported by precheck and merge the PR to mainline, here is a partial list in yesterday: https://github.com/apache/incubator-nuttx/pull/471/checks https://github.com/apache/incubator-nuttx/pull/479/checks https://github.com/apache/incubator-nuttx/pull/471/checks ... Do you think we can get the buildable mainline with this process if we consider NuttX contain more than 1000 option and nobody can try all possible config/host locally? Actually, I don't care who fix the style issue in the original code base(contributor or committer), the more important is that all PR must pass the precheck and nobody modify the PR locally before merging.. Since the precheck is ready now, I would suggest that we modify the current workflow to better utilize the precheck: 1.Don't create a local PR branch, make some modification and merge the local PR branch directly to master. This process totally bypass the precheck and may make the mainline unbuildable. 2.Don't merge the PR before precheck report the pass, I would prefer we disable merge button during the checking/building. 3.Consider the current codebase isn't style clean and nxstyle isn't in the perfect state, I suggest nxstyle just check the modified part. Item 3 is just a suggestion, but please don't modify/merge PR when the precheck report error, otherwise we can never get a stable mainline ready for release. > Just my 2 cents. > > BR, > > Alan > > On 3/7/20, Xiang Xiao <xiaoxiang781...@gmail.com> wrote: > > Hi all, > > The precheck ensure the whole file content comform to the coding > > style, this strategy has several problems: > > 1.Many source file in mainline already violate the coding style > > 2.nxstyle frequently generate the false alarm in the current stage > > How about we let precheck just ensure the modified line don't violate > > the coding standards? > > I am asking this question because: > > 1.The change in PR 471 has a very good shape: > > https://github.com/apache/incubator-nuttx/pull/471/files > > but the whole file precheck complain there are many errors: > > > > https://github.com/apache/incubator-nuttx/pull/471/checks?check_run_id=492244725 > > It is unfair to require the contributor to fix the issue not made by > > them. > > 2.Most PR will fail at precheck stage due to item 1 and then block the > > more important build test. > > > > Thanks > > Xiang > >
