diff mbox series

[OpenWrt-Devel] build: refactor JSON info files

Message ID 20200215232702.2778489-1-mail@aparcar.org
State Changes Requested
Delegated to: Petr Štetiar
Headers show
Series [OpenWrt-Devel] build: refactor JSON info files | expand

Commit Message

Paul Spooren Feb. 15, 2020, 11:27 p.m. UTC 
JSON info files contain machine readable information of built profiles
and resulting images. These files where added via 881ed09ee6e2. They are
useful for firmware wizards and script checking for reproducibility.

Currently all JSON files are stored next to the built images, resulting
in up to 168 individual files for the ath79/generic target.

This PR refactors the JSON creation to store individual files in
$(KDIR)/tmp and create an single overview file called `profiles.json` in
the target dir.

As before, this creation is enabled by default only for the BUILDBOT.

To archive the previous behaviour the option JSON_INDIVIDUAL_JSON_INFO
can be set.

Signed-off-by: Paul Spooren <mail@aparcar.org>
---
 Makefile                            |  6 ++++++
 config/Config-build.in              | 24 +++++++++++++++++----
 include/image.mk                    |  9 +++++---
 scripts/json_overview_image_info.py | 33 +++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 7 deletions(-)
 create mode 100755 scripts/json_overview_image_info.py

Comments

Petr Štetiar Feb. 28, 2020, 9:48 p.m. UTC | #1 
Paul Spooren <mail@aparcar.org> [2020-02-15 13:27:03]:

Hi,

> This PR refactors the JSON creation to store individual files in
> $(KDIR)/tmp and create an single overview file called `profiles.json` in
> the target dir.
> 
> As before, this creation is enabled by default only for the BUILDBOT.
> 
> To archive the previous behaviour the option JSON_INDIVIDUAL_JSON_INFO
> can be set.

Why do we need to preserve that previous behaviour?

FYI those individual files are broken[1] on some buildhosts (8C/16T, E5-2650) anyway:

 Traceback (most recent call last):
  File "/builds/ynezz/openwrt/scripts/json_add_image_info.py", line 49, in <module>
    device_info = json.load(json_file)
  File "/usr/lib/python3.5/json/__init__.py", line 268, in load
    parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
  File "/usr/lib/python3.5/json/__init__.py", line 319, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python3.5/json/decoder.py", line 342, in decode
    raise JSONDecodeError("Extra data", s, end)
 json.decoder.JSONDecodeError: Extra data: line 35 column 2 (char 823)

Would be nice to fix that and making this errors fatal so it's more visible and
can be fixed sooner then later.

> +jsonoverviewimageinfo: FORCE

nitpickbutyoucanusedashorundescoreifyouwantforsuchmultiwordtargetsormakethetargetnameshorter :)

1. https://ynezz.gitlab.io/-/openwrt/-/jobs/447337426/artifacts/logs/target/linux/install.txt

-- ynezz
Paul Spooren Feb. 28, 2020, 11:09 p.m. UTC | #2 
Hi,

>> This PR refactors the JSON creation to store individual files in
>> $(KDIR)/tmp and create an single overview file called `profiles.json` in
>> the target dir.
>>
>> As before, this creation is enabled by default only for the BUILDBOT.
>>
>> To archive the previous behaviour the option JSON_INDIVIDUAL_JSON_INFO
>> can be set.
> Why do we need to preserve that previous behaviour?

Two reasons:

* We have to create those files anyway before a merge because image 
creation happens in parallel, so no single overview can be added to. If 
this is a wrong assumption please step in
* For ImageBuilders it is convenient to have a per profile file. It does 
not add much complexity as the file is simply copied.

I'd be in favor of keeping it if you wouldn't mind. It would be disabled 
for buildbots and users per default.

> FYI those individual files are broken[1] on some buildhosts (8C/16T, E5-2650) anyway:
>
>   Traceback (most recent call last):
>    File "/builds/ynezz/openwrt/scripts/json_add_image_info.py", line 49, in <module>
>      device_info = json.load(json_file)
>    File "/usr/lib/python3.5/json/__init__.py", line 268, in load
>      parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
>    File "/usr/lib/python3.5/json/__init__.py", line 319, in loads
>      return _default_decoder.decode(s)
>    File "/usr/lib/python3.5/json/decoder.py", line 342, in decode
>      raise JSONDecodeError("Extra data", s, end)
>   json.decoder.JSONDecodeError: Extra data: line 35 column 2 (char 823)
>
> Would be nice to fix that and making this errors fatal so it's more visible and
> can be fixed sooner then later.
I'm looking into this issue but haven't figured it out yet. Either the 
JSON generation or file writing is broken. The odd thing about it is 
that it also happens if a file is opened only once (as in only a single 
image is created), so it is unlikely an issue with parallel writing into 
a file... If anyone has ideas, please share.
>> +jsonoverviewimageinfo: FORCE
> nitpickbutyoucanusedashorundescoreifyouwantforsuchmultiwordtargetsormakethetargetnameshorter :)
No this is a value comment and no nit pick, I just assumed wrongly that 
dashes/underscores should be avoided as they're not used in the Makefile 
so far.
> 1. https://ynezz.gitlab.io/-/openwrt/-/jobs/447337426/artifacts/logs/target/linux/install.txt

Best,
Paul
Petr Štetiar Feb. 28, 2020, 11:41 p.m. UTC | #3 
Paul Spooren <mail@aparcar.org> [2020-02-28 13:09:46]:

> > > To archive the previous behaviour the option JSON_INDIVIDUAL_JSON_INFO
> > > can be set.
> > Why do we need to preserve that previous behaviour?
> 
> Two reasons:
> 
> * We have to create those files anyway before a merge because image creation
> happens in parallel, so no single overview can be added to. If this is a
> wrong assumption please step in

Ok, but we don't need two different config options for that. One for single
file JSON output and another one for amalgamated JSON.

> * For ImageBuilders it is convenient to have a per profile file.

I don't follow here.

> I'd be in favor of keeping it if you wouldn't mind. It would be disabled for
> buildbots and users per default.

I don't like an idea of having *two* different options for *one* functionality
for no reason. Initial idea was JSON for online builders, so thats *one* config
option to me.

> I'm looking into this issue but haven't figured it out yet. Either the JSON
> generation or file writing is broken. The odd thing about it is that it also
> happens if a file is opened only once (as in only a single image is
> created), so it is unlikely an issue with parallel writing into a file... If
> anyone has ideas, please share.

What about some graceful error handling, outputing the broken JSON file? Maybe
it's another Docker filesystem related hiccup?

-- ynezz
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 181c33b180..6bc5b487b6 100644
--- a/Makefile
+++ b/Makefile
@@ -96,6 +96,11 @@  buildversion: FORCE
 feedsversion: FORCE
 	$(SCRIPT_DIR)/feeds list -fs > $(BIN_DIR)/feeds.buildinfo
 
+jsonoverviewimageinfo: FORCE
+	INPUT_DIR=$(BUILD_DIR)/linux-$(BOARD)$(if $(SUBTARGET),_$(SUBTARGET))/tmp \
+		TARGET_DIR=$(BIN_DIR) \
+		$(SCRIPT_DIR)/json_overview_image_info.py
+
 diffconfig: FORCE
 	mkdir -p $(BIN_DIR)
 	$(SCRIPT_DIR)/diffconfig.sh > $(BIN_DIR)/config.buildinfo
@@ -108,6 +113,7 @@  prepare: .config $(tools/stamp-compile) $(toolchain/stamp-compile)
 
 world: prepare $(target/stamp-compile) $(package/stamp-compile) $(package/stamp-install) $(target/stamp-install) FORCE
 	$(_SINGLE)$(SUBMAKE) -r package/index
+	$(if $(CONFIG_JSON_OVERVIEW_IMAGE_INFO),$(_SINGLE)$(SUBMAKE) -r jsonoverviewimageinfo)
 	$(_SINGLE)$(SUBMAKE) -r checksum
 
 .PHONY: clean dirclean prereq prepare world package/symlinks package/symlinks-install package/symlinks-clean
diff --git a/config/Config-build.in b/config/Config-build.in
index 59dfaea8bb..6f381eb250 100644
--- a/config/Config-build.in
+++ b/config/Config-build.in
@@ -7,12 +7,28 @@ 
 
 menu "Global build settings"
 
-	config JSON_ADD_IMAGE_INFO
-		bool "Create JSON info files per build image"
+	config JSON_OVERVIEW_IMAGE_INFO
+		bool "Create JSON info file overview per target"
 		default BUILDBOT
+		select JSON_CREATE_IMAGE_INFO
 		help
-		  The JSON info files contain information about the device and
-		  build images, stored next to the firmware images.
+		  Create a JSON info file called profiles.json in the target
+		  directory containing machine readable list of built profiles
+		  and resulting images.
+
+	config JSON_INDIVIDUAL_IMAGE_INFO
+		bool "Create individual JSON info files per build profile"
+		default n
+		select JSON_CREATE_IMAGE_INFO
+		help
+		  The JSON info files contain information about built profiles
+		  and resulting images. Activating this option creates a self
+		  containing info file next to the images in the target
+		  directory.
+
+	config JSON_CREATE_IMAGE_INFO
+		bool
+		default n
 
 	config ALL_NONSHARED
 		bool "Select all target specific packages by default"
diff --git a/include/image.mk b/include/image.mk
index fd04d4020b..375d865cd0 100644
--- a/include/image.mk
+++ b/include/image.mk
@@ -568,9 +568,9 @@  define Device/Build/image
 
   $(BIN_DIR)/$(call IMAGE_NAME,$(1),$(2)): $(KDIR)/tmp/$(call IMAGE_NAME,$(1),$(2))
 	cp $$^ $$@
-	$(if $(CONFIG_JSON_ADD_IMAGE_INFO), \
+	$(if $(CONFIG_JSON_CREATE_IMAGE_INFO), \
 		DEVICE_ID="$(DEVICE_NAME)" \
-		BIN_DIR="$(BIN_DIR)" \
+		BIN_DIR="$(KDIR)/tmp" \
 		IMAGE_NAME="$(IMAGE_NAME)" \
 		IMAGE_TYPE=$(word 1,$(subst ., ,$(2))) \
 		IMAGE_PREFIX="$(IMAGE_PREFIX)" \
@@ -594,6 +594,9 @@  define Device/Build/image
 		SUPPORTED_DEVICES="$(SUPPORTED_DEVICES)" \
 		$(TOPDIR)/scripts/json_add_image_info.py \
 	)
+	$(if $(CONFIG_JSON_INDIVIDUAL_IMAGE_INFO), \
+		cp $(KDIR)/tmp/$(IMAGE_PREFIX).json $(BIN_DIR)/$(IMAGE_PREFIX).json \
+	)
 
 endef
 
@@ -612,7 +615,7 @@  define Device/Build/artifact
 endef
 
 define Device/Build
-  $(shell rm -f $(BIN_DIR)/$(IMG_PREFIX)-$(1).json)
+  $(shell rm -f $(KDIR)/$(IMG_PREFIX)-$(1).json)
 
   $(if $(CONFIG_TARGET_ROOTFS_INITRAMFS),$(call Device/Build/initramfs,$(1)))
   $(call Device/Build/kernel,$(1))
diff --git a/scripts/json_overview_image_info.py b/scripts/json_overview_image_info.py
new file mode 100755
index 0000000000..bba13dd80c
--- /dev/null
+++ b/scripts/json_overview_image_info.py
@@ -0,0 +1,33 @@ 
+#!/usr/bin/env python3
+
+import json
+from pathlib import Path
+from os import getenv
+
+target_dir = Path(getenv("TARGET_DIR"))
+input_dir = Path(getenv("INPUT_DIR", target_dir))
+
+output_json = {}
+
+assert target_dir, "Target directory required"
+
+for json_file in input_dir.glob("*.json"):
+    profile_info = json.loads(json_file.read_text())
+    if not output_json:
+        output_json = {
+            "metadata_version": 1,
+            "target": profile_info["target"],
+            "version_commit": profile_info["version_commit"],
+            "version_number": profile_info["version_number"],
+            "profiles": {},
+        }
+
+    output_json["profiles"][profile_info["id"]] = {
+        "supported_devices": profile_info["supported_devices"],
+        "images": profile_info["images"],
+        "titles": profile_info["titles"],
+    }
+
+Path(target_dir / "profiles.json").write_text(
+    json.dumps(output_json, sort_keys=True, indent="  ")
+)