Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
[TEST] abuild: Default to !CHROMEOS
Change-Id: I9ae4bc22f70be5b8d8efa35f0cc8aa2d36a5531a Signed-off-by: Nico Huber nico.h@gmx.de --- M util/abuild/abuild 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/49056/1
diff --git a/util/abuild/abuild b/util/abuild/abuild index 408de12..59da54c 100755 --- a/util/abuild/abuild +++ b/util/abuild/abuild @@ -655,7 +655,7 @@ clean_objs=false verboseopt='V=0' customizing="" -configoptions="" +configoptions="# CONFIG_CHROMEOS is not set\n" # testclass needs to be undefined if not used for variable expansion to work unset testclass while true ; do
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 1:
board.GOOGLE_ARCADA board.GOOGLE_BOTEN board.GOOGLE_BOTEN_LEGACY board.GOOGLE_DEDEDE board.GOOGLE_DELBIN board.GOOGLE_DRALLION board.GOOGLE_DRAWCIA board.GOOGLE_DRAWCIA_LEGACY board.GOOGLE_ELDRID board.GOOGLE_ELEMI board.GOOGLE_GALTIC board.GOOGLE_HALVOR board.GOOGLE_LANTIS board.GOOGLE_LINDAR board.GOOGLE_MADOO board.GOOGLE_MAGOLOR board.GOOGLE_MALEFOR board.GOOGLE_METAKNIGHT board.GOOGLE_SARIEN board.GOOGLE_SASUKE board.GOOGLE_STORO board.GOOGLE_TERRADOR board.GOOGLE_TODOR board.GOOGLE_TRONDO board.GOOGLE_VOEMA board.GOOGLE_VOLTEER board.GOOGLE_VOLTEER2 board.GOOGLE_VOLTEER2_TI50 board.GOOGLE_VOXEL board.GOOGLE_WADDLEDEE board.GOOGLE_WADDLEDOO board.GOOGLE_WHEELIE board.INTEL_ADLRVP_P board.INTEL_JASPERLAKE_RVP board.INTEL_JASPERLAKE_RVP_EXT_EC board.INTEL_TGLRVP_UP3 board.INTEL_TGLRVP_UP4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4: Code-Review+1
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST] can probably drop now?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
can probably drop now?
Well, I don't know why CHROMEOS would default to y.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
can probably drop now?
Is this supposed to be submitted? I'm kinda confused as to the point of this patch, isn't !CHROMEOS already the default? I think all this would do is to change the specific builds in configs/ that manually set CONFIG_CHROMEOS=y, but do we actually want that? And if we want that, why not just change the config files themselves rather than doing this roundabout thing?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
Is this supposed to be submitted? I'm kinda confused as to the point of this patch, isn't !CHROMEOS […]
This change was helpful to catch some dependencies that would force CHROMEOS to be enabled for some boards. I don't think this change should be submitted, though.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
Well, I don't know why CHROMEOS would default to y.
I meant drop the [TEST] part, as highlighted :)
Is this supposed to be submitted? I'm kinda confused as to the point of this patch, isn't !CHROMEOS already the default?
the point is to force abuild to test the !CHROMEOS case for boards which default to CHROMEOS=y without having to create special configs for them. Ideally, all of those boards should have that default removed, but this serves as a backstop against new ones being added with it defaulting to Y and then the !CHROMEOS case not being tested and winding up broken, as the 4 preceding patches address
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
This change was helpful to catch some dependencies that would force CHROMEOS to be enabled for some boards. I don't think this change should be submitted, though.
and the boards that explicitly set CHROMEOS=y in their Kconfig
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/4//COMMIT_MSG@7 PS4, Line 7: [TEST]
This change was helpful to catch some dependencies that would force CHROMEOS to be enabled for som […]
Okay, I noticed now that a bunch of mainboards override the default for config CHROMEOS. I agree that should get fixed.
Is that maybe something we can put into kconfig_lint instead? It's already parsing the defaults of all Kconfigs, it hopefully shouldn't be so hard to just check if something there overrides CHROMEOS. I think that would be a cleaner way to add that check than this.
Attention is currently required from: Matt DeVillier, Julius Werner, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/comment/8259dfe6_63142999 PS4, Line 7: [TEST]
Okay, I noticed now that a bunch of mainboards override the default for config CHROMEOS. […]
As long as nobody relies on the `default y` in newer mainboards, I would also vote for adapting the linter.
I don't remember if there was ever a rule against defaulting to CHROMEOS that was just forgotten, or if it's just implied by the way abuild is written.
Attention is currently required from: Nico Huber, Matt DeVillier, Angel Pons. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/comment/373bd4f7_e27f8259 PS4, Line 7: [TEST]
As long as nobody relies on the `default y` in newer mainboards, I would also […]
I mean, I think that generally the "feature Kconfigs" should always have the same defaults everywhere and there's no reason for any board to impose a different one from the rest. It's just a matter of consistency. CHROMEOS is one example but things like COLLECT_TIMESTAMPS, USE_OPTION_TABLE, BOOTSPLASH_IMAGE, TPM_MEASURED_BOOT, VBOOT, FATAL_ASSERTS, the various DEBUG_xxx configs... that's all stuff that is a fundamentally board-agnostic feature decision and no board should be overriding defaults for them. (Boards can use depends or select to constrain the value if it just doesn't work otherwise on that platform, but they shouldn't be overriding defaults.)
Maybe we can say that the general rule should be that mainboards and SoCs may only override defaults from options that cannot be changed in menuconfig? Or would that be too restrictive?
Attention is currently required from: Nico Huber, Matt DeVillier, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49056 )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49056/comment/43fda0a3_399e485b PS4, Line 7: [TEST]
I mean, I think that generally the "feature Kconfigs" should always have the same defaults everywher […]
While it might make sense to override some user-visible option default values on a per-mainboard basis (e.g. disable postcodes over LPC for a mainboard that does not expose LPC on any header or connector), I agree that it's most often a bad idea.
About CHROMEOS, I think Jenkins should fail to verify a change that overrides the default value. A way to implement this check would be to ensure the default config for all mainboards never contains CONFIG_CHROMEOS=y.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/49056?usp=email )
Change subject: [TEST] abuild: Default to !CHROMEOS ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Martin L Roth (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)