[OpenWrt-Devel] [PATCH v2] build: refactor JSON info files to `profiles.json`

[Paul Spooren]

>> This problem occurs also fox x86, the problem is that the image function is
>> not properly called. Maybe because IMX6 only offer a default target but no
>> profiles, resulting in an empty profiles.json file - I think. I started
>> (based on Lynxis draft) reworking the x86 so it creates also JSON files[0].
> This doesn't make much sense to me. It's now possible to create separate JSON
> files[1], but not the amalgamated one? There is something wrong with this
> reasoning.
>> I'd be in favor reworking (unifying) the target specific code instead of
>> extending the script logic to handle corner cases. At least if reworking is
>> something what should be done anyway.
> Sure, patches are always welcome and unification is great and needed, but I
> think, that fixing that INPUT_DIR variable of yours would be lower hanging
> fruit.

Sorry I made a bad assumption here. x86 has to be reworked but imx6 is 
fine. Problem was that $(KDIR) lacks the subtarget suffix (as imx6 got 
none), so the JSON files weren't found. I changed in v4 the JSON storage 
path which makes it independent of subtargets. Good catch!


>> I tried adding things like `|| exit 1` but make stubbornly keeps going. Can
>> you help me out here please?
> Looking at the log I see (ignored):
>
>   /openwrt.git/scripts/json_add_image_info.py: line 1: borken: command not found
>   Makefile:235: recipe for target '/openwrt.git/bin/targets/imx6/generic/openwrt-imx6-apalis-squashfs.sysupgrade.bin' failed
>   make[5]: [/openwrt.git/bin/targets/imx6/generic/openwrt-imx6-apalis-squashfs.sysupgrade.bin] Error 127 (ignored)
>
> Which leads to following in the include/image.mk:
>
>   .IGNORE: $(BIN_DIR)/$(call IMAGE_NAME,$(1),$(2))

I changed the code so that failing image creation is fill ignored while 
failing image creation actually exists. In include/image.mk

-  .IGNORE: $(BIN_DIR)/$(call IMAGE_NAME,$(1),$(2))

[...]

    $(BIN_DIR)/$(call IMAGE_NAME,$(1),$(2)): $(KDIR)/tmp/$(call 
IMAGE_NAME,$(1),$(2))
-       cp $$^ $$@
+       -cp $$^ $$@

The prefixed dash ignores a failure.

In scripts/json_add_image_info.py

+image_file = bin_dir / getenv("IMAGE_NAME")
+if not image_file.is_file():
+    print("Skip JSON creation for non existing image ", image_file)
+    quit()

This way the Python scripts detects an image wasn't copied and quits. If 
the scripts crashes for any other reason, the make process exists with 1.

Is that what you're looking for?

>>>> +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": {},
>>>> +        }
>>> I'm not a Pythonista, but perhaps you want to init the output_json dict just a
>>> few lines above and get rid of that unnecesary if.
>> The `profile_info` variable is only available after reading the first JSON
>> profile and therefore in the loop.
> Indeed, making me confused, hiding unhandled corner case as well, like for
> example now, where you're producing `{}` instead of expected or rather correct
> `{'profiles': {}, 'metadata_version': 1}`.
>
> I would write it as following:
>
> output_json = {
>      "profiles": {},
>      "metadata_version": 1,
> }
>
> def add_target_info(ojs, js):
>      if 'target' in ojs:
>          return
>
>      ojs["target"] = js["target"]
>      ojs["version_commit"] = js["version_commit"]
>      ojs["version_number"] = js["version_number"]
>
> def add_profile_info(ojs, js):
>      profile = {
>          "images": js["images"],
>          "titles": js["titles"],
>          "supported_devices": js["supported_devices"]
>      }
>      ojs["profiles"][js["id"]] = profile
>
> for json_file in Path(os.getcwd()).glob("*.json"):
>      js = json.loads(json_file.read_text())
>      add_target_info(output_json, js)
>      add_profile_info(output_json, js)
>
> print(json.dumps(output_json, separators=(",", ":")))

Another good catch, thanks! The code is a bit less Pythonic but more 
C'ish, but why not.

+output_json = {"metadata_version": 1, "profiles": {}}
+
+
+def add_target_info(ojs: dict, js: dict):
+    if "target" in ojs:
+        return
+
+    ojs["target"] = js["target"]
+    ojs["version_code"] = js["version_code"]
+    ojs["version_number"] = js["version_number"]
+
+
+def add_profile_info(ojs: dict, js: dict):
+    ojs["profiles"][js["id"]] = {
+        "images": js["images"],
+        "titles": js["titles"],
+        "supported_devices": js["supported_devices"],
      }

+
+for json_file in input_dir.glob("*.json"):
+    js = json.loads(json_file.read_text())
+    add_target_info(output_json, js)
+    add_profile_info(output_json, js)
+
  Path(target_dir / "profiles.json").write_text(
-    json.dumps(output_json, sort_keys=True, indent="  ")
+    json.dumps(output_json, sort_keys=True, separators=(",", ":"))

  )

>> json.dumps(output_json, sort_keys=True, indent="  ")
> BTW I've just noticed that, why do you need to produce human readable JSON?
No it doesn't. Firefox e.g. renders JSON files automatically so we don't 
need to waste space. Thanks.
> 1. https://downloads.openwrt.org/snapshots/targets/imx6/generic/openwrt-imx6-apalis.json

If you're okay with these changes I'll create a v4. Thanks for your 
patience!

Best,
Paul


_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel