-
Notifications
You must be signed in to change notification settings - Fork 20
Consolidate board specific files #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aeb0d4a to
8c27119
Compare
|
Another part that will be missing to be a bit more generic is to detect which boards (device trees) a kernel carries, and only build flat images for these boards. But that can land separately, this PR is already helping consolidating board definitions in a single place. |
The flash debos recipe creates flash-ufs and flash-emmc files, use these to select the artifacts to include in the UFS and eMMC tarballs. Signed-off-by: Loïc Minier <[email protected]>
Having all dtbs available makes development easier and board support filtering can happen when generating flashable assets. Signed-off-by: Loïc Minier <[email protected]>
8c27119 to
9554158
Compare
Test jobs for commit 8c27119 |
Test jobs for commit 9554158 |
basak-qcom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the dtbs.tar.gz, flash-emmc.tar.gz and flash-ufs.tar.gz output artifacts and they all look fine. One minor comment inline (no need to change or re-review).
| -C "${latest_kernel}" \ | ||
| --transform='s|^\./||' \ | ||
| -cvzf "$ARTIFACTDIR/dtbs.tar.gz" \ | ||
| . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to an ugly ./ directory as a tar entry, which seems barely worth the --transform. I guess this is because --transform won't eliminate something entirely.
I'm not sure why you want to strip out ./, but given that you do and assuming you don't mind about missing dotfiles, maybe (cd "${latest_kernel}" && tar -cvzf ... *) instead? To include dotfiles there's either shopt -s dotfiles (but I think that's bash only) or something like (cd "${latest_kernel}" && find -mindepth 1 -print0|cut -zd/ -f2-|tar --verbatim-files-from --files-from=- --null -cvzf ...)
This is probably not worth adjusting unless you want to :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you guessed, I added the transform to avoid ./ in pathnames in the tar archive. This is to preserve the same path structure as in the previous version, were specific relative dtb files were specified (without ./).
The dtb pathnames are specified in a different file, flash.yaml, as part of the board specific metadata; it would be ugly to have ./ names in there, but I guess I could also prepend it when calling tar -x. The ./ felt like gratuitous pollution of dtbs.tar.gz, so I found this --transform way of stripping them.
I can switch to cd $dir && tar instead of -C + --transform, I guess this is easier to read but I wanted to avoid relying on globing in case for a weird reason a kernel comes with no dtb (which isn't really an use case, and then perhaps the directory wouldn't exist either).
I don't think there are dotfiles in there to grab; the find | cut | tar would be painful to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the glob is empty, then it will remain by default and the tar will then fail I think - perhaps that's what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, trying to move to cd, the two things that scratch me with these few lines are:
- ARTIFACTDIR not being documented as absolute in the debos docs (probably it is and will be forever)
- my desire to use linux-version to list kernel versions (as captured in dtbs.tar.gz should use linux-version list #22), but that requires running a command in chroot: true mode and that prevents access to ARTIFACTDIR and I don't really want to break this sequence in 3 actions
So if you don't mind, I'd prefer keeping tar in the current directory with the slightly unusual --transform, it's confirmed to work by you.
Consolidate logic for boards in a single place:
Fixes: #144