Re: Commit-Regeln für die Firmware

Do Jul 22 12:36:44 CEST 2021

Hallo zusammen,

ich möchte diese Idee noch einmal entstauben und neu bewerten.
Ich bin damals wohl irgendwie nicht dazu gekommen, auf diese Mail zu antworten. Möglicherweise hatte ich da grade viel um die Ohren.

Aktuell ist das Problem wieder relativ akut, dass viele eigentlich wichtige Patches liegen bleiben, keine Reviewer finden, und entsprechend nicht applied werden können. Meine eigenen Änderungen kann ich selbst ja nicht reviewen.

Das ganze bremst die Entwicklung aktuell ziemlich, obwohl ich eigentlich gerne möglichst zügig auf ein DSA Release (oder zumindest beta) zusteuern möchte, da die EdgeRouter X im aktuellen Zustand mit dem bekannten Ethernet Bug kaum zu gebrauchen sind.
Ebenso wird auch das Performanceproblem von babeld immer problematischer, speziell auf langsameren Routern.

Wenn jeder Entwicklungsschritt erst wochenlang herumliegt, und dann am Ende mangels Review doch nicht applied werden kann, macht das Firmware-Frickeln auch nur sehr eingeschränkt Spaß.
Vielleicht müssen wir unsere doch recht hohen Qualitätsstandards (ein, am besten zwei Reviews für jede(!) Änderung, ..) ein wenig zurückschrauben, da wir dafür scheinbar nicht ausreichend Entwicklerkapazität (oder Interesse?) haben. Von einer Firmware, deren Entwicklung stagniert, hat auch niemand etwas.

Daher möchte ich die Idee von Adrian noch einmal Vorschlagen:
> Ich möchte hier eine Diskussion anregen, ob für uns eine Öffnung zu entsprechenden Regeln mit mehr Macht und Flexibilität für den einzelnen Maintainer gewünscht ist.
> In meinen Augen kann dies sinnvoll sein, sofern bestimmte Bedingungen erfüllt werden:
> 
> 1. Bestimmte minimale Grundkriterien für einen akzeptablen Commit. Für mich wären das ein ganz knapper Text der Form von Commit Title/Message/xxx-by sowie sinnvolle Aufteilung von Commits irgendwo fixiert.
> 
> 2. Ein klar definierter Personenkreis, der Commit-Rechte besitzt.
> 
> 3. Bei allen Beteiligten wird ein verantwortungsvoller Umgang vorausgesetzt, Änderungen sollten entsprechend durchdacht und ggf. getestet sein. Je unsicherer, desto eher macht es Sinn, nachzufragen oder auf Reviews zu warten.
Ich wäre sehr dafür den aktiven Entwicklern entsprechende Rechte einzuräumen, so dass die Entwicklung - speziell auch von einfacheren (aufräum-) Patches - beschleunigt werden kann. Besonders dann, wenn gerade nur wenig Review-Kapazität verfügbar ist.

Gruß
Fabian

On 06.04.20 22:01, mail at adrianschmutzler.de wrote:
> Hallo,
> 
> zwei Wochen sind rum, und der einzige, der geantwortet hat, war Christian.
> 
> Da Christian im Wesentlichen die Meinung vertritt, es so zu belassen, wie es im Moment ist, und sonst keine Meinung vorhanden ist, ist die Sache damit für mich erledigt.
> 
> Beste Grüße
> 
> Adrian
> 
>> -----Original Message-----
>> From: Christian Dresel [mailto:fff at chrisi01.de]
>> Sent: Sonntag, 22. März 2020 21:53
>> To: mail at adrianschmutzler.de; franken-dev at freifunk.net
>> Subject: Re: Commit-Regeln für die Firmware
>>
>> Hi
>>
>> gleich vorweg, ich hab keine Erfahrung aus anderen Projekten und beziehe
>> meine Aussagen von hörensagen und was ich in letzter Zeit persönlich
>> einfach passend fand.
>>
>> On 22.03.20 21:28, mail at adrianschmutzler.de wrote:
>>> Hallo zusammen,
>>>
>>> schon seit einiger Zeit machen sich ja verschiedene Leute Gedanken, ob
>> wir mit der relativ kleinen Zahl an Entwicklern ggf. etwas an den
>> ungeschriebenen Regeln ändern sollten, wann ein Patch committed werden
>> darf.
>>>
>>> Bisher habe ich den modus operandi so verstanden (und praktiziert), dass
>> ein Patch nur dann applied wird, wenn mindestens ein Review vorhanden ist
>> (ggf. auch mehr).
>>> Dies stellt einerseits sicher, dass immer vier Augen einen Patch geprüft
>> haben, führt auf der anderen Seite aber auch dazu, dass die Entwicklung
>> gebremst wird und bei trivialen Patches im Prinzip auch Ressourcen
>> verschwendet werden.
>>
>> ich hab das mittlerweile so gehandhabt, das ich so Kleinigkeiten die auf der
>> ML gelandet sind kurz angeguckt und dann einfach ein review dagegen
>> geworfen habe. Siehe die 2 Sachen heute. Ich finde es irgendwie doch gut,
>> wenn einfach noch ein 2 Paar Augen drüber guckt. Mir wurde immer wieder
>> erklärt, ein Review ist im Prinzip nix anderes wie:
>> "Ich hab mal über den Code geguckt, weiß zum größtenteil was er tut, er
>> passt ins Konzept und es ist kein quatsch"
>> Review heißt nicht:
>> "Ich bin mir sicher das der Code absolut perfekt tut, hab ihn gebaut und
>> geflasht und keinerlei Sicherheitslücken enthält und hab ihn komplett
>> durchgetestet"
>>
>> Das hab ich in letzter Zeit auch so gelebt und finde es ansich recht passend.
>> So ein "ich hab da ein Komma hinzugefügt" ist zwar nur ne Kleinigkeit, da
>> kann ich aber auch mal eben $Mod+P drücken mein Gruß Christian drunter
>> klopfen und rausschicken, dauert keine Minute und es hat ein 2. Paar Augen
>> einfach mal drauf geguckt. Sonst hat man so ne schwimmende Grenze:
>> "Da ein Komma geht schon"
>> "Da ein Satz geht schon"
>> "Da eine Codezeile ist ja auch nicht viel"
>> "Da die ganze Funktion auf den Kopf stellen ist doch nur ne Kleinigkeit"
>> "..."
>> also wo zieht man dann die Grenze?
>>
>>>
>>> Auf der anderen Seite habe ich bei OpenWrt ein System kennengelernt,
>> dass dem einzelnen Committer deutlich mehr Macht verleiht. Im
>> Wesentlichen entscheidet dort jeder selbst, ob er einen Patch direkt applied
>> oder vorher zum Review an die Mailingliste schickt. Je nach Schwere des
>> Eingriffs wartet man dann eine gewisse Zeit, und applied den Patch
>> entweder wenn kein Veto kommt oder nach entsprechendem Acked-by
>> oder Review.
>>> Sowohl die Entscheidung, die Liste zu befragen, als auch die Wartezeit/das
>> Kriterium für den Merge entscheidet jeder selbst. Wer bloß der Sortierung
>> oder einen Tippfehler ändert, kann dies einfach reinwerfen, wer
>> configurehood komplett umbauen will, würde wohl besser nachfragen. Ggf.
>> reicht z.B. auch ein Acked-by aus, wenn man sich mit dem Code sicher ist,
>> aber nicht weiß, ob das Feature gewünscht ist etc.
>>
>> dieses acked-by hab ich eher so verwendet:
>> "Commitmsg beschreibt etwas was ich haben will, den Code hab ich mir aber
>> nicht weiter angeguckt"
>>
>> Ich finde nicht, das es für ein applien ausreichen sollte.
>>
>> Es gibt ja dann noch das tested-by, das heißt für mich dann, der Code ist
>> komplett durchgetestet und funktioniert einwandfrei.
>>
>> Ich würde also dahingehen das:
>> Zum applien ein reviewed-by weiterhin nötig ist. Aber hier auch nochmal
>> klarstellen das ein reviewed-by eben kein "Code komplett durchgetestet
>> und tut" ist sondern das was ich oben bereits geschrieben habe.
>> Im Gegenzug fände ich es gut, wenn sehr komplexe oder kritische umbauten
>> (muss jeder für sich entscheiden was darunter fällt) vllt. sogar ein tested-by
>> haben sollten. Das würde für mich mindestens heißen: "Ich hab es gebaut,
>> geflasht und der Code tut augenscheinlich das was zu erwarten war" (kommt
>> am Ende natürlich auf den Einzelfall an, bei einem Komma in der README ist
>> es Käse, bei hochziehen von OpenWRT z.b. wäre es sinnvoll es zumindest für
>> alle Geräte durchzubauen und auf ein Gerät zu flashen mMn.). Dieses
>> tested-by kann ja auch der Patchersteller machen, ich denke wer ein
>> komplexen Patch baut, wird ihn zumindest auch einmal testen oder? Wenn
>> dann noch jemand ein reviewed-by (Bedingungen siehe
>> oben) drüber wirft, wäre das ok zum applien.
>>
>> Hätte z.b. der Patchersteller das OpenWRT 19.7 mal mit allen devices
>> durchgebaut, wäre wohl aufgefallen, dass das cp vom ER-X(-SFP) failed (soll
>> jetzt keine Anschuldigung sein bitte nicht übel nehmen aber es passt gerade
>> gut, nur ein Beispiel was besser gewesen wäre wenn man meinen oberen
>> Fall leben würde).
>>
>> Oder anderes Beispiel:
>> Wer ein neues Device himzufügt, wird es ja mindestens 1x testen. Auch hier
>> kann der Ersteller des Patches dann eigentlich gleich ein tested-by dran
>> hängen, ein Reviewer guckt kurz drüber ob er echt nur das device hinzufügt
>> und nicht vllt. irgendwo versucht groben Unfug mit reinzuschmuggeln, macht
>> ein reviewed-by und alles ist gut.
>>
>>>
>>> Ich möchte hier eine Diskussion anregen, ob für uns eine Öffnung zu
>> entsprechenden Regeln mit mehr Macht und Flexibilität für den einzelnen
>> Maintainer gewünscht ist.
>>> In meinen Augen kann dies sinnvoll sein, sofern bestimmte Bedingungen
>> erfüllt werden:
>>>
>>> 1. Bestimmte minimale Grundkriterien für einen akzeptablen Commit. Für
>> mich wären das ein ganz knapper Text der Form von Commit
>> Title/Message/xxx-by sowie sinnvolle Aufteilung von Commits irgendwo
>> fixiert.
>>>
>>> 2. Ein klar definierter Personenkreis, der Commit-Rechte besitzt.
>>> Zugriff auf das firmware-repo haben z.Zt:
>>> RedDog
>>> Steffen Pankratz kratz00
>>> fblaese
>>> meskal
>>> mayosemmel
>>> adrianschmutzler
>>
>> hier würde ich anregen die Leute mal nochmal persönlich anzuschreiben
>> (falls sie jetzt nicht schon hier antworten) und fragen ob sie das überhaupt
>> noch machen möchten und wenn keine Antwort kommt runternehmen.
>>
>>
>>>
>>> 3. Bei allen Beteiligten wird ein verantwortungsvoller Umgang
>> vorausgesetzt, Änderungen sollten entsprechend durchdacht und ggf.
>> getestet sein. Je unsicherer, desto eher macht es Sinn, nachzufragen oder
>> auf Reviews zu warten.
>>
>> 4.
>> Wie wollen wir es mit Klarnamenpflicht bei signed-off und reviewed-by/etc.
>> halten? Die Diskussion kam ja auch schonmal auf. Ich bin hier sehr unsicher.
>>
>> Gruß
>>
>> Christian
>>
>>>
>>> Über Meinungen würde ich mich freuen.
>>>
>>> Beste Grüße
>>>
>>> Adrian
>>>

-------------- nächster Teil --------------
Ein Dateianhang mit Binärdaten wurde abgetrennt...
Dateiname   : OpenPGP_signature
Dateityp    : application/pgp-signature
Dateigröße  : 833 bytes
Beschreibung: OpenPGP digital signature
URL         : <https://lists.freifunk.net/pipermail/franken-dev-freifunk.net/attachments/20210722/2338618e/attachment.sig>