Change in coreboot[master]: sconfig: Switch to getopt

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

sconfig: Switch to getopt

Instead of positional arguments switch sconfig to use getopt and pass
the arguments as options in the build system.  This will make it easier
to add additional options.

Change-Id: I431633781e80362e086c000b7108191b5b01aa9d
Signed-off-by: Duncan Laurie 
---
M Makefile.inc
M util/sconfig/main.c
2 files changed, 50 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44035/1

diff --git a/Makefile.inc b/Makefile.inc
index a51b73d..dc1c6e0 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -592,19 +592,23 @@
 # Creation of these is architecture and mainboard independent
 DEVICETREE_FILE := $(src)/mainboard/$(MAINBOARDDIR)/$(CONFIG_DEVICETREE)

+SCONFIG_OPTIONS := --mainboard_devtree=$(DEVICETREE_FILE)
+
 ifneq ($(CONFIG_OVERRIDE_DEVICETREE),)
-
 OVERRIDE_DEVICETREE_FILE := $(src)/mainboard/$(MAINBOARDDIR)/$(CONFIG_OVERRIDE_DEVICETREE)
-
+SCONFIG_OPTIONS += --override_devtree=$(OVERRIDE_DEVICETREE_FILE)
 endif

DEVICETREE_STATIC_C := $(obj)/mainboard/$(MAINBOARDDIR)/static.c
+SCONFIG_OPTIONS += --output_c=$(DEVICETREE_STATIC_C)
+
 DEVICETREE_STATIC_H := $(obj)/static.h
+SCONFIG_OPTIONS += --output_h=$(DEVICETREE_STATIC_H)

$(DEVICETREE_STATIC_C): $(DEVICETREE_FILE) $(OVERRIDE_DEVICETREE_FILE) $(objutil)/sconfig/sconfig
    @printf "    SCONFIG    $(subst $(src)/,,$(<))\n"
    mkdir -p $(dir $(DEVICETREE_STATIC_C))
-	$(objutil)/sconfig/sconfig $(DEVICETREE_FILE) $(DEVICETREE_STATIC_C) $(DEVICETREE_STATIC_H) $(OVERRIDE_DEVICETREE_FILE)
+	$(objutil)/sconfig/sconfig $(SCONFIG_OPTIONS)

ramstage-y+=$(DEVICETREE_STATIC_C)
 romstage-y+=$(DEVICETREE_STATIC_C)
diff --git a/util/sconfig/main.c b/util/sconfig/main.c
index 170acad..66f9cbf 100644
--- a/util/sconfig/main.c
+++ b/util/sconfig/main.c
@@ -2,6 +2,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <ctype.h>
+#include <getopt.h>
 /* stat.h needs to be included before commonlib/helpers.h to avoid errors.*/
 #include <sys/stat.h>
 #include <commonlib/helpers.h>
@@ -1309,21 +1310,14 @@

static void usage(void)
 {
-	printf("usage: sconfig devicetree_file output_file header_file [override_devicetree_file]\n");
+	printf("usage: sconfig <options>\n");
+	printf("  -c | --output_c          : Output static.c file (required)\n");
+	printf("  -r | --output_h          : Header static.h file (required)\n");
+	printf("  -m | --mainboard_devtree : Mainboard devicetree file (required)\n");
+	printf("  -o | --override_devtree  : Override devicetree file (optional)\n");
    exit(1);
 }

-enum {
-	DEVICEFILE_ARG = 1,
-	OUTPUTFILE_ARG,
-	HEADERFILE_ARG,
-	OVERRIDE_DEVICEFILE_ARG,
-};
-
-#define MANDATORY_ARG_COUNT		4
-#define OPTIONAL_ARG_COUNT		1
-#define TOTAL_ARG_COUNT		(MANDATORY_ARG_COUNT + OPTIONAL_ARG_COUNT)
-
 static void parse_devicetree(const char *file, struct bus *parent)
 {
    FILE *filec = fopen(file, "r");
@@ -1674,18 +1668,47 @@

int main(int argc, char **argv)
 {
-	if ((argc < MANDATORY_ARG_COUNT) || (argc > TOTAL_ARG_COUNT))
-		usage();
+	static const struct option long_options[] = {
+		{ "mainboard_devtree", 1, NULL, 'm' },
+		{ "override_devtree", 1, NULL, 'o' },
+		{ "output_c", 1, NULL, 'c' },
+		{ "output_h", 1, NULL, 'r' },
+		{ "help", 1, NULL, 'h' },
+		{ }
+	};
+	const char *override_devtree = NULL;
+	const char *base_devtree = NULL;
+	const char *outputc = NULL;
+	const char *outputh = NULL;
+	int opt, option_index;

-	const char *base_devtree = argv[DEVICEFILE_ARG];
-	const char *outputc = argv[OUTPUTFILE_ARG];
-	const char *outputh = argv[HEADERFILE_ARG];
-	const char *override_devtree;
+	while ((opt = getopt_long(argc, argv, "m:o:c:r:h", long_options,
+				  &option_index)) != EOF) {
+		switch (opt) {
+		case 'm':
+			base_devtree = strdup(optarg);
+			break;
+		case 'o':
+			override_devtree = strdup(optarg);
+			break;
+		case 'c':
+			outputc = strdup(optarg);
+			break;
+		case 'r':
+			outputh = strdup(optarg);
+			break;
+		case 'h':
+		default:
+			usage();
+		}
+	}
+
+	if (!base_devtree || !outputc || !outputh)
+		usage();

parse_devicetree(base_devtree, &base_root_bus);

-	if (argc == TOTAL_ARG_COUNT) {
-		override_devtree = argv[OVERRIDE_DEVICEFILE_ARG];
+	if (override_devtree) {
    	parse_devicetree(override_devtree, &override_root_bus);

if (!dev_has_children(&override_root_dev)) {

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 1: Code-Review+2

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 2: Code-Review+1

Do we need to include getopt.c for macos / BSD / Windows?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c 
File util/sconfig/main.c:

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c@1314 
PS2, Line 1314: printf("  -c | --output_c          : Output static.c file (required)\n");
              : 	printf("  -r | --output_h          : Header static.h file (required)\n");
              : 	printf("  -m | --mainboard_devtree : Mainboard devicetree file (required)\n");
              : 	printf("  -o | --override_devtree  : Override devicetree file (optional)\n");
These are *paths* to files, right?

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 2:

(1 comment)
> Patch Set 2: Code-Review+1
>
> Do we need to include getopt.c for macos / BSD / Windows?

Isn't getopt just part of libc?

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c 
File util/sconfig/main.c:

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c@1314 
PS2, Line 1314: printf("  -c | --output_c          : Output static.c file (required)\n");
              : 	printf("  -r | --output_h          : Header static.h file (required)\n");
              : 	printf("  -m | --mainboard_devtree : Mainboard devicetree file (required)\n");
              : 	printf("  -o | --override_devtree  : Override devicetree file (optional)\n");
> These are *paths* to files, right?

Correct, point taken 😄

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded a new patch set (#4) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

sconfig: Switch to getopt

Instead of positional arguments switch sconfig to use getopt and pass
the arguments as options in the build system.  This will make it easier
to add additional options.

Change-Id: I431633781e80362e086c000b7108191b5b01aa9d
Signed-off-by: Duncan Laurie 
---
M Makefile.inc
M util/sconfig/main.c
2 files changed, 50 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44035/4

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c 
File util/sconfig/main.c:

https://review.coreboot.org/c/coreboot/+/44035/2/util/sconfig/main.c@1314 
PS2, Line 1314: printf("  -c | --output_c          : Output static.c file (required)\n");
              : 	printf("  -r | --output_h          : Header static.h file (required)\n");
              : 	printf("  -m | --mainboard_devtree : Mainboard devicetree file (required)\n");
              : 	printf("  -o | --override_devtree  : Override devicetree file (optional)\n");
> Correct, point taken 😄

Done

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:

I think most environments have a getopt implementation already, glibc does, freebsd does, if there are any I am unaware of that we don't and we break the environment for someone, I will be more than happy to put up a GPL'd getopt.c and add it to the makefile.

Any problems with that approach?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:
> I think most environments have a getopt implementation already, glibc does, freebsd does, if there are any I am unaware of that we don't and we break the environment for someone, I will be more than happy to put up a GPL'd getopt.c and add it to the makefile.
>
> Any problems with that approach?

If in doubt, assume what other tools assume, e.g. cbfstool :)

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:
> Patch Set 4:
> > I think most environments have a getopt implementation already, glibc does, freebsd does, if there are any I am unaware of that we don't and we break the environment for someone, I will be more than happy to put up a GPL'd getopt.c and add it to the makefile.
> >
> > Any problems with that approach?
>
> If in doubt, assume what other tools assume, e.g. cbfstool :)

My only concern is that sconfig is required for coreboot itself to build, whereas the others aren't.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:
> > > I think most environments have a getopt implementation already, glibc does, freebsd does, if there are any I am unaware of that we don't and we break the environment for someone, I will be more than happy to put up a GPL'd getopt.c and add it to the makefile.
> > >
> > > Any problems with that approach?
> >
> > If in doubt, assume what other tools assume, e.g. cbfstool :)
>
> My only concern is that sconfig is required for coreboot itself to build, whereas the others aren't.

But they are. cbfstool, rmodtool... good luck building without them ;)

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4:
> Patch Set 4:
> > > > I think most environments have a getopt implementation already, glibc does, freebsd does, if there are any I am unaware of that we don't and we break the environment for someone, I will be more than happy to put up a GPL'd getopt.c and add it to the makefile.
> > > >
> > > > Any problems with that approach?
> > >
> > > If in doubt, assume what other tools assume, e.g. cbfstool :)
> >
> > My only concern is that sconfig is required for coreboot itself to build, whereas the others aren't.
>
> But they are. cbfstool, rmodtool... good luck building without them ;)

Ok, point taken 😊

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 4: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

sconfig: Switch to getopt

Instead of positional arguments switch sconfig to use getopt and pass
the arguments as options in the build system.  This will make it easier
to add additional options.

Change-Id: I431633781e80362e086c000b7108191b5b01aa9d
Signed-off-by: Duncan Laurie 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44035
Tested-by: build bot (Jenkins) 
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Angel Pons 
---
M Makefile.inc
M util/sconfig/main.c
2 files changed, 50 insertions(+), 23 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/Makefile.inc b/Makefile.inc
index ab0e5bb..a43de5e 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -595,19 +595,23 @@
 # Creation of these is architecture and mainboard independent
 DEVICETREE_FILE := $(src)/mainboard/$(MAINBOARDDIR)/$(CONFIG_DEVICETREE)

+SCONFIG_OPTIONS := --mainboard_devtree=$(DEVICETREE_FILE)
+
 ifneq ($(CONFIG_OVERRIDE_DEVICETREE),)
-
 OVERRIDE_DEVICETREE_FILE := $(src)/mainboard/$(MAINBOARDDIR)/$(CONFIG_OVERRIDE_DEVICETREE)
-
+SCONFIG_OPTIONS += --override_devtree=$(OVERRIDE_DEVICETREE_FILE)
 endif

DEVICETREE_STATIC_C := $(obj)/mainboard/$(MAINBOARDDIR)/static.c
+SCONFIG_OPTIONS += --output_c=$(DEVICETREE_STATIC_C)
+
 DEVICETREE_STATIC_H := $(obj)/static.h
+SCONFIG_OPTIONS += --output_h=$(DEVICETREE_STATIC_H)

$(DEVICETREE_STATIC_C): $(DEVICETREE_FILE) $(OVERRIDE_DEVICETREE_FILE) $(objutil)/sconfig/sconfig
    @printf "    SCONFIG    $(subst $(src)/,,$(<))\n"
    mkdir -p $(dir $(DEVICETREE_STATIC_C))
-	$(objutil)/sconfig/sconfig $(DEVICETREE_FILE) $(DEVICETREE_STATIC_C) $(DEVICETREE_STATIC_H) $(OVERRIDE_DEVICETREE_FILE)
+	$(objutil)/sconfig/sconfig $(SCONFIG_OPTIONS)

ramstage-y+=$(DEVICETREE_STATIC_C)
 romstage-y+=$(DEVICETREE_STATIC_C)
diff --git a/util/sconfig/main.c b/util/sconfig/main.c
index 170acad..f561806 100644
--- a/util/sconfig/main.c
+++ b/util/sconfig/main.c
@@ -2,6 +2,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <ctype.h>
+#include <getopt.h>
 /* stat.h needs to be included before commonlib/helpers.h to avoid errors.*/
 #include <sys/stat.h>
 #include <commonlib/helpers.h>
@@ -1309,21 +1310,14 @@

static void usage(void)
 {
-	printf("usage: sconfig devicetree_file output_file header_file [override_devicetree_file]\n");
+	printf("usage: sconfig <options>\n");
+	printf("  -c | --output_c          : Path to output static.c file (required)\n");
+	printf("  -r | --output_h          : Path to header static.h file (required)\n");
+	printf("  -m | --mainboard_devtree : Path to mainboard devicetree file (required)\n");
+	printf("  -o | --override_devtree  : Path to override devicetree file (optional)\n");
    exit(1);
 }

-enum {
-	DEVICEFILE_ARG = 1,
-	OUTPUTFILE_ARG,
-	HEADERFILE_ARG,
-	OVERRIDE_DEVICEFILE_ARG,
-};
-
-#define MANDATORY_ARG_COUNT		4
-#define OPTIONAL_ARG_COUNT		1
-#define TOTAL_ARG_COUNT		(MANDATORY_ARG_COUNT + OPTIONAL_ARG_COUNT)
-
 static void parse_devicetree(const char *file, struct bus *parent)
 {
    FILE *filec = fopen(file, "r");
@@ -1674,18 +1668,47 @@

int main(int argc, char **argv)
 {
-	if ((argc < MANDATORY_ARG_COUNT) || (argc > TOTAL_ARG_COUNT))
-		usage();
+	static const struct option long_options[] = {
+		{ "mainboard_devtree", 1, NULL, 'm' },
+		{ "override_devtree", 1, NULL, 'o' },
+		{ "output_c", 1, NULL, 'c' },
+		{ "output_h", 1, NULL, 'r' },
+		{ "help", 1, NULL, 'h' },
+		{ }
+	};
+	const char *override_devtree = NULL;
+	const char *base_devtree = NULL;
+	const char *outputc = NULL;
+	const char *outputh = NULL;
+	int opt, option_index;

-	const char *base_devtree = argv[DEVICEFILE_ARG];
-	const char *outputc = argv[OUTPUTFILE_ARG];
-	const char *outputh = argv[HEADERFILE_ARG];
-	const char *override_devtree;
+	while ((opt = getopt_long(argc, argv, "m:o:c:r:h", long_options,
+				  &option_index)) != EOF) {
+		switch (opt) {
+		case 'm':
+			base_devtree = strdup(optarg);
+			break;
+		case 'o':
+			override_devtree = strdup(optarg);
+			break;
+		case 'c':
+			outputc = strdup(optarg);
+			break;
+		case 'r':
+			outputh = strdup(optarg);
+			break;
+		case 'h':
+		default:
+			usage();
+		}
+	}
+
+	if (!base_devtree || !outputc || !outputh)
+		usage();

parse_devicetree(base_devtree, &base_root_bus);

-	if (argc == TOTAL_ARG_COUNT) {
-		override_devtree = argv[OVERRIDE_DEVICEFILE_ARG];
+	if (override_devtree) {
    	parse_devicetree(override_devtree, &override_root_bus);

if (!dev_has_children(&override_root_dev)) {

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20001
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20000
"QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19999
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19998
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19997
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20005
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20004
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20003
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20002

Please note: This test is under development and might not be accurate at all!

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44035 )

Change subject: sconfig: Switch to getopt
......................................................................

Patch Set 5:

Looks like one of the merged patches just hasn't made it through to chromium coreboot yet, I'll cherry-pick them from here.