Re: Commit-Regeln für die Firmware

Christian Dresel fff at chrisi01.de
So Mär 22 21:53:12 CET 2020


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
> 


Mehr Informationen über die Mailingliste franken-dev