Add Device Tree Overlay support for U-Boot raw disk images, plus other features for Common Torizon#153
Conversation
|
I see that I forgot to add the tests for the new features. I'll put this PR in draft until I add them. |
2ff48c9 to
91d0478
Compare
|
OK, I've added the tests. It's ready for review |
jsrc27
left a comment
jsrc27
left a comment
There was a problem hiding this comment.
Overall looks okay, no major issues. I did comment on some minor stuff though.
rborn-tx
left a comment
rborn-tx
left a comment
There was a problem hiding this comment.
Unfortunately I didn't have time to look into the tests, but the implementation looks great.
I've raised a few questions for discussion.
| download_dir = None | ||
| if "image-download-policy" in props and props["image-download-policy"] == "use-cache": | ||
| if "image-download-dir" in props: | ||
| if not os.path.isdir(props["image-download-dir"]): | ||
| os.makedirs(props["image-download-dir"]) | ||
| common.set_output_ownership(props["image-download-dir"], set_parents=True) | ||
| download_dir = props["image-download-dir"] | ||
| else: | ||
| download_dir = os.getcwd() |
There was a problem hiding this comment.
I'm glad to see you're clearing an important TODO of adding support for caching input images.
Since this is a whole new feature, I think it's worth discussing its behavior.
If I get it correctly from this stretch of code, the downloads directory would be used only when the user specified the download-policy "use-cache". From this I understand that the download directory is seen as a synonym to the cache directory, right?
Some questions come to mind:
- If the user specified a download directory, shouldn't we always use it, regardless of a download-policy?
- What do we lose if we don't have a download-policy at all?
I raise these questions to see if we could simplify the UX even further.
Say we didn't have the policy I'd expect a behavior like this:
- If the user specified the directory, we would always store the file in that directory.
- If the file already exists and it matches the user-specified hash (when the
remoteproperty was used), then do not download the file. - If the file already exists and the above condition was not true then check if the existing file hash matches the one on the server; if so, do not download it again.
- If none of the previous two conditions was true, download the file.
There was a problem hiding this comment.
- If the user specified a download directory, shouldn't we always use it, regardless of a download-policy?
- What do we lose if we don't have a download-policy at all?
I raise these questions to see if we could simplify the UX even further.
Hmm right, that is more or less the approach I did for images download: the image file would only be locally saved if the user explicitly passes the new --download-dir argument.
I agree that just having image-download-dir would be simpler. Thanks, I'll apply your suggestion.
| """Perform the 'dt status' command.""" | ||
|
|
||
| images_unpack_executed() | ||
| img_arch = get_arch_from_ostree() |
There was a problem hiding this comment.
Have you considered some way to determine if U-Boot is being used as bootloader? If we had a reliable way to do so, we could make the conditions independent of the machine and dependent on the bootloader, which I believe is what matters here, right?
There was a problem hiding this comment.
I considered checking for U-Boot here, but I imagined that using the target architecture of the OS image would be a more generic way to filter out device tree/device tree overlay support, as x86_64 machines don't usually support them.
Thinking now that would probably be a valid check to make if DT/DTO was not supported at all for x86_64, which is not necessarily the case.
So it makes more sense to have a whitelist i.e. a list of supported bootloaders rather than trying to blacklist unsupported bootloaders, target architectures, etc., which is basically what you suggested.
The deploy command uses a backend/common.py function called get_image_bootloader() to determine the bootloader of the unpacked OS image. It's a function that I made more or less based on the method used by libostree to determine the bootloader (U-Boot and GRUB2). I'm thinking on using that here as well.
|
I forgot to mention in the review: the commit message title: seems a bit odd to me (particularly the terms |
|
Sure, |
Also implement TEZI and raw image checksum verification using a hash obtained from the X-Checksum-Sha256 HTTP header or provided by the user. Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
Add new optional 'image-download-dir' field in the 'input' section to allow users to save input images downloaded via the 'remote' and 'toradex-feed' methods to a given relative path. If this field is not provided, then the behavior stays the same as before: the image is downloaded to a temporary directory and it isn't saved. Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
Use the same method in 'build' to download Torizon OS images from the Toradex servers. Previously 'images download' had its own implementation to download the images. Add new '--download-dir' optional argument to set the directory path to save the image file. If not provided, the behavior is the same as the default one: the image is downloaded to a temporary directory and won't be saved. 'images download' now obtain most of image info of the remote device by reading the contents of /etc/os-release. Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
jsrc27
left a comment
jsrc27
left a comment
There was a problem hiding this comment.
@lucas-akira From my side I don't have any further comments. I think all of @rborn-tx comments have been addressed as well. If you're fine with it we can merge now since he's out for the next couple of weeks. Or do you want to wait?
|
@jsrc27 In the |
Add WIC/raw disk (.img) image support for the following commands: - dto apply - dto list - dto status - dto remove The respective Device Tree Overlay customization options of the 'build' command (add, remove, clear) are also supported. This support only applies for OS images that use U-Boot as the bootloader. Related-to: TCB-865 Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
Stop execution of 'dt' and 'dto' commands, along with their equivalent 'build' features, if the unpacked OS image doesn't have a supported bootloader for DT/DTO customization (currently only U-Boot). The bootloader of the image is determined using the 'deploy' get_image_bootloader() function. Related-to: TCB-865 Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
|
OK, I've replaced the arch check with the bootloader one and it's ready for review. I also had to do a small change to the |
Related-to: TCB-865 Signed-off-by: Lucas Akira Morishita <lucas.morishita@toradex.com>
3 participants
Add support to apply, remove and check applied device tree overlays for raw disk images using U-Boot , either using the
dtocommand or using the correspondingtcbuild.yamloption for thebuildcommand.Add support to download supported Common Torizon images in raw disk format from the Toradex servers with
images downloadand with the input option of thebuildcommand.Also implement input image (TEZI and raw disk) checksum verification for
images downloadandbuild.See the commit messages for more details.
Related-to: TCB-865