Hi Jiri,
On Mon, 2024-12-02 at 16:25 +0100, Jiri Hnidek wrote:
> I made some changes in PR #3 (mostly related to code style).
> I added more error messages.
> I also discovered a bug in dmidecode.c and fixed it in a separate
> commit.
Thank you very much for your work. I'm reviewing your PR now.
> Open questions:
> 1) Should be support for output to JSON format (requiting json-c) enabled or
> disabled by default?
If this can be implemented easily in the Makefile, I would enable JSON
output by default, as I think most distributions will want to include
it, and probably most users as well. Something like the following may
work, although I'm not sure how portable it is:
--- a/Makefile
+++ b/Makefile
@@ -21,17 +21,23 @@ CFLAGS ?= -O2
CFLAGS += -W -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith -Wcast-qual \
-Wcast-align -Wwrite-strings -Wmissing-prototypes -Winline -Wundef
+# By default we include support for JSON output, can be overridden on
+# the command line
+WITH_JSON ?= 1
+
# Let lseek and mmap support 64-bit wide offsets
CFLAGS += -D_FILE_OFFSET_BITS=64
#CFLAGS += -DBIGENDIAN
#CFLAGS += -DALIGNMENT_WORKAROUND
-#CFLAGS += -DWITH_JSON_C
# Pass linker flags here (can be set from environment too)
LDFLAGS ?=
-#LDFLAGS += -ljson-c
+ifeq ($(WITH_JSON),1)
+CFLAGS += -DWITH_JSON_C
+LDFLAGS += -ljson-c
+endif
DESTDIR =
prefix = /usr/local
With this, just "make" will include JSON output, while "make
WITH_JSON=0" will not.
> 2) When support for JSON output format is disabled, should the -j and --json
> CLI option be present?
> If yes, should we print some error message, when -j or --json is used.
> Something like: "dmidecode build without JSON support"
> If no, then I think that It could be confusing to have same version of
> dmidecode with different
> CLI option set on different platforms and distributions.
I don't have a strong opinion on this, but I suppose it's more simple
to include the options always. The overhead should be marginal, and
this has the advantage to let the user know that the possibility
exists, so they can look into enabling it if they need it.
> I'm sorry that I wasn't more active last week, but I was busy with other
> projects.
If you are sorry for one week, then what should *I* say for one year?
;-)
--
Jean Delvare
SUSE L3 Support
