image: be more consistent in image naming

[pprindeville]

Image names are not handled in a consistent way. Here we attempt to rectify this.

[mpratt14]

Hi Philip,

in another PR I was attempting to add a metadata entry that's unique on a per-image basis
for example a JSON line something like
"image": "factory",

The original purpose of doing this was rejected, but it may be useful in the future for other things.

Originally I tried to use the variable IMAGE_TYPE by defining it at the top of Device/Build/image the same way it is defined in the middle of Device/Build/image and then call it from the metadata_json definition in image-commands.mk
IMAGE_TYPE=$(word 1,$(subst ., ,$(2)))

but I couldn't get it to work....I would get the same value for IMAGE_TYPE in both images

maybe you would know how to do that properly
but if that's not in the scope of this PR you can disregard

[adschm]

Hi, I'm sorry, but this looks wrong to me.

IMG_PREFIX is device-independent, so it can't be set based on a device-dependent variable.

This might work if you only build one device, but not for more than one.

[adschm]

FYI: ef2cb85

[aparcar]

"image": "factory",

This looks fine to me, ideally the sysupgrade prints a warning if you flash a factory image aka you''re loosing the config and it should also complain if you install a ext4 sysupgrade over a squashfs image. What were the objections? If there is no PR or mail thread, please open a PR to discuss details.

[mpratt14]

@aparcar the original PR was for verifying the sysupgrade.tgz config tarball by putting the metadata inside of it. Dropped due to it being seen as an unnecessary workaround, adding too much complexity to build system, and the issue I mentioned.

However your idea to have it display a warning message is a nice usage of the metadata part

[pprindeville]

"image": "factory",

This looks fine to me, ideally the sysupgrade prints a warning if you flash a factory image aka you''re loosing the config and it should also complain if you install a ext4 sysupgrade over a squashfs image. What were the objections? If there is no PR or mail thread, please open a PR to discuss details.

See the thread with subject Inconsistencies in include/image.mk

[mpratt14]

See the thread with subject Inconsistencies in include/image.mk

@pprindeville we were talking about another topic, not this one, sorry for the confusion

[adschm]

BTW this lacks a commit message as well. Thus, apart from it violating the formal requirements, the PR also fails to explain why the change would be an improvement. (if it works at all)

[pprindeville]

Hi, I'm sorry, but this looks wrong to me.

IMG_PREFIX is device-independent, so it can't be set based on a device-dependent variable.

This might work if you only build one device, but not for more than one.

I'm not sure I understand what "it's device-independent" means. I can't run a x86 image on an ARMv7 architecture, or ARM LE on an ARM BE platform, etc.

And one image might be built for a machine with 256MB of RAM and 512MB of flash, and another image for 1GB of RAM and 4GB of Flash...

All images are device-dependent to some degree. I'm not following.

[pprindeville]

FYI: ef2cb85

Yup, saw that and made a tweak to DEVICE_IMG_PREFIX.

[pprindeville]

Pulling in @dangowrt

[adschm]

Hi, I'm sorry, but this looks wrong to me.
IMG_PREFIX is device-independent, so it can't be set based on a device-dependent variable.
This might work if you only build one device, but not for more than one.

I'm not sure I understand what "it's device-independent" means. I can't run a x86 image on an ARMv7 architecture, or ARM LE on an ARM BE platform, etc.

And one image might be built for a machine with 256MB of RAM and 512MB of flash, and another image for 1GB of RAM and 4GB of Flash...

All images are device-dependent to some degree. I'm not following.

Config for building in OpenWrt has two different levels: One is the level of targets/subtargets, where kernel config is set, features are selected (e.g. ramdisk usbgadget small_flash nand ...), and - partly as a consequence of the former - architecture etc. is defined. All packages are build once per target, unless they are shared and are target-invariant (then they are built only once for the entire tree).

The second level are devices (or "profiles", as they have been called in earlier times). Devices put together pre-built components (kernel, packages, ...) with some image headers etc. into images. Devices do not modify kernel or package contents.

For each of these two levels, there is a specific set of variables. "Normal" (read =default) variables only have one value per (sub)target (e.g. CPU_TYPE). If you modified them for one device, all devices would have the same single value after the Makefiles have been parsed (which is the last value set). Then there are device variables. These variables do have different values for each device (if properly set up). This is essentially everything that has been added to DEVICE_VARS or DEFAULT_DEVICE_VARS (e.g. DEVICE_PACKAGES or BLOCKSIZE, with a very small number of exceptions).
Thus, while one can read from target ("normal") variables in a device-/image-specific context, you should never write to them. On the other side, using device-specific variables in a target context makes no sense at all.

Since this is rather confusing even to experienced OpenWrt developers, we typically prefixiall/most device-dependent variables with "DEVICE_" unless it's obvious they are device-dependent (thus, the difference between IMG_PREFIX and DEVICE_IMG_PREFIX, where the latter already contains the device name).

What you try do here now is essentially taking a device-dependent variable, PROFILE_SANITIZED, from a context where it was only evaluated (i.e. read) per image (Image/Manifest, Image/Build) and start to assign a "normal" (=target-level) variable IMG_PREFIX based on its value. Note that, as discussed above, this variable will then only have one single value for all devices in the target and thus the proposed change will not work if you build more than one device. (Probably even with TARGET_MULTI_PROFILE enabled and one device, but that's speculation).

Apart from that, it's hard to do any further discussion or pointers for improvement, since you never considered it necessary to explain what's the actual purpose (and more important, benefit) of this change. So, let's start with that and tell us what you actually want to achieve, and maybe we can work out something that actually works and does not break anything else on the way.

[pprindeville]

I've already explained this in a few different places.

At run-time, on the target, I want to be able to reconstruct this from include/image.mk:

IMG_PREFIX:=$(VERSION_DIST_SANITIZED)-$(IMG_PREFIX_VERNUM)$(IMG_PREFIX_VERCODE)$(IMG_PREFIX_EXTRA)$(BOARD)$(if $(SUBTARGET),-$(SUBTARGET))$(if $(PROFILE_SANITIZED),-$(PROFILE_SANITIZED))

to figure out what image to download if I've got a bunch of image names to glob against on a mirror.

That is, I want to be able to figure out what the most approximate (newer) image to the one the box is currently running might be.

This shouldn't be this hard.

And I want to do it in a way that maps to/from the way that the image name was computed at build time. Not some possibly incorrect inference based on scripts run on the target that might end up with the wrong result (like board_detect and /etc/board.json).

[pprindeville]

The second level are devices (or "profiles", as they have been called in earlier times). Devices put together pre-built components (kernel, packages, ...) with some image headers etc. into images. Devices do not modify kernel or package contents.

There's an easy fix for this: don't have /etc/os-release be part of any package, in that case.

Have it be part of the image finalization in include/image.mk's Device/Build/image or Device/Build phases.

[adschm]

I have commented on the subject of bringing device names into /etc/os-release in the other thread.

Still, I do not understand how this connects to this PR and why we would need this variable rearrangement presented here for that. Since I also still believe this is just plainly wrong, I'm closing this PR now.

If I'm mistaken in my judgement, please explain why this mixture of variables is still valid, and I will reopen. Apart from that, I also still don't understand what the immediate purpose of the changes in this PR would be. This is something that would need to be explained in a commit message, which hasn't been done so far either.

[pprindeville]

@adschm Sorry, the last set of comments weren't very relevant to this PR (I was thinking about a different one).

This is organizational. It's trying to simplify the creation of image names (which are profile specific) is done in a single consistent way. The device naming $(DEVICE_IMG_PREFIX) is effectively unchanged, as it's seeded with $(IMG_PREFIX_PROFILELESS), and no references to $(IMG_PREFIX) occurs in any Device/* macros. The only time $(IMG_PREFIX) is used is by itself in Image/Manifest.

The following are unchanged:

The derived value of $(IMG_ROOTFS) is used in Image/Build/targz and Image/Build/cpiogz, and the expansion is identical.

The derived value of $(IMG_COMBINED) doesn't seem to be used anywhere I can tell.

From what I can tell, Build/install-dtb should be using the profile name, but previously wasn't.

From what you say, it seems like all the references to $(IMG_PREFIX) in target/ are incorrect and should instead be $(DEVICE_IMG_PREFIX) instead!!! Can you please confirm?

[pprindeville]

@adschm You asked how this PR is relevant. I tried to answer that above.

[adschm]

I read it a second time, but still don't understand your point.

[dangowrt]

@adschm x86/generic and x86/64 are just a very special cases, completely different from all the rest of OpenWrt. Have that in mind when looking at this issue, because from what I understand it's pointing to x86-specifics. In short: `/tmp/sysinfo/*` is populated from DMI, and that has no relationship the image filename. Actually you can run x86/generic and x86/64 images on all of those (PCEngines, SuperMicro, ...) boards. See https://github.com/openwrt/openwrt/blob/master/target/linux/x86/base-files/lib/preinit/01_sysinfo#L10 So other than on almost all other targets, _profiles_ in this context are merely a combination of packages installed by default, the image is always _generic_. I have worked around that to get `auc` to work on x86 recently openwrt/packages@02bc1fc#diff-65b4b8439d7a00eb7a5ccdddf7a7caa27b9b51d5569ff6f91eea5ade66143fdaR559 and since I did that I do understand the point of this PR to some degree, or at least I understand that things are a bit inconsistent... From my perspective, differentiation of x86-boards should remain to happen only at run-time when ever possible, as this is how x86 (as in: the original IBM PC, but also ACPI, DMI, UEFI, ...) generally works. There are no different device-trees, kernels, vendor-specific flash formats, ... which need special care and would legitimate different image filenames. Maybe we should reflect that in `/tmp/sysinfo` in a more consistent way, as `board_name` right now means two different things and `profile` yet another thing and they just happen to correlate on embedded targets...

[pprindeville]

What @dangowrt said: whatever consistency of naming there is, doesn't apply to x86 in any useful way.

Daniel: any thoughts on the PR content itself?

[adschm]

What @dangowrt said: whatever consistency of naming there is, doesn't apply to x86 in any useful way.

Yes, but that's not a reason to break everything else just to make x86 work in your specific usecase.