On Thu, Feb 6, 2025 at 9:30 AM raiden00pl <raiden0...@gmail.com> wrote: > >On Thu, Feb 6, 2025 at 8:45 AM Michal Lenc <michall...@seznam.cz> wrote: > > Perhaps I would limit this to larger commits only (and all affecting > Build / Kernel / common arch code of course). GitHub bot tells you the > size of the pull request, so we can draw some line according to that. > And maybe it could also automatically set the mandatory number of > reviewers. This way simple changes/fixes/additions to BSPs or MCU > drivers would be handled faster. > > +1. Not all PRs have the same importance and impact. For PR that > has a high impact on other users, 4 reviewers make sense. > For PR that has low impact, addressing new features or newly > developed architectures this is overkill. I could list more cases where > such an approach doesn't make sense considering the resources > we have. We'll end up being flooded with PRs without approvals, > which will discourage both contributors and reviewers.
Yeah, I meant complex and possible breaking changes not a typos fixes, sorry ~16..20h a day in front of a computer daily.. I need some sleep ;-) I am just not sure if git repos like github does not have static settings for N positive reviews and one negative to block merge.. something like our Apache voting system. On the other hand it is not that hard to gather 4 reviewers, and this will for sure improve cross-check and block too quick breaking merges, we can easily request additional reviewers with one click on github, and this could motivate activity from the community so currently most active reviewers (i.e. Xiang) does not feel alone, even small input counts and then becomes a habit :-) I dont understand one thing. Why reviewer needs to be a committer from start? I think someone should be reviewer in the first place and after some time when things work fine a committer may be granted for the reviewer :-) This could also motivate for reviews to be then promoted to committer..? :-) > Using github labels in this case can help, but the automation of > this is very limited by the tool. It's not really possible to create > complicated dependencies for labels, or I haven't figured out how to > do it. We can introduce a new label "4 approvals", but it's assignment > will have to be done manually by the PR creator or reviewer. The problem right now is that we set rules that we don't obey ourselves :-P That is proper git message and PR description.. and most important test logs!! These are veeery often non-existent and we really need to work on that :-) I would say "zero-trust" approach could be of most benefit because it would be committer who is responsible for proving that their change is well tested and works on a real world hardware. We need to start thinking system wide :-) To be honest myself at first for some time I did build and test each PR myself.. but this is too much manual work.. and not always necessary for small commits.. but we should have this working culture.. at least when things go bad we look into the git log and then PR and at least know what was reasoning behind a problem and how to fix it instead reinventing the wheel that cost time! :-) Again. Its not about making things hard. We work on automations, what we have is getting improved, something new will show up for help. I know everyone works in good will and best interest. But we should have this standard quality filter so people respect our master because its not a messy playground and the project can be trusted in the long term :-) > We could also add labels for individual chips and separate them from > labels for code common to the architecture (like "Arch: arm" and > "Arch: STM32"), this would greatly help with judging the importance > of PR. Unfortunately, implementing this is a long and boring work. Yeah. That could be useful for automation but lets put time there only when necessary, time is most precious, thanks! :-) > > By Architecture, do you mean common architecture code or even MCU > specific code? I am not sure if describing every change to a single MCU > on a mailing list is beneficial, I am afraid it will just flood the list > and no one will read it. GitHub issues seem more suitable for this. By Architecture I mean changing tens or hundreds of files in one change because of change of API that would for sure result in compatibility problems and wild outrage of the crowds :D If you make this kind of change why not discuss it before? Why not provide metrics of improvement along the change? Runtime logs from real hardware that it is obvious these need to be created and verified to prove even internally nothing is broken. I think ultimately every change in nuttx repo should be tested in pair with nuttx-apps and on hardware.. thus idea for at least standard self-test.. maybe except trivial obvious typo fixes.. but look if you change some MCU code that would impact lots of boards, if you change headers it could impact even more, not to mention kernel stuff that impacts everything. Like when you create or update a driver why would you not want to provide testing logs if you have them anyway.. you have them right?? :D Raiden, its like you want to update STM code to get rid of obsolete pin definitions. You communicated this long before change is planned, before you even started working on this, asked folks for help, proposed to implement it in a next major release (13) that is not yet even planned to maximize compatibility of 12 line, because you understand this will break stuff for someone. And this IS the "MCU specific code" right? Maybe Im not that much of a cultural guy but culture of working (in terms of organization) comes to my mind. We should focus on quality not quantity. And most of all long-term self-compatibility :-) Thank you and have a good day folks :-) Tomek -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
