Message ID | 20190903120951.3318-1-raphael.melotte@essensium.com |
---|---|
State | Rejected |
Headers | show |
Series | [1/1] support/scripts/genimage.sh: allow setting rootpath from parameters. | expand |
Hello Raphael, On Tue, 3 Sep 2019 14:09:51 +0200 raphael.melotte@essensium.com wrote: > From: Raphaël Mélotte <raphael.melotte@essensium.com> > > Previously the rootpath was always set to $TARGET_DIR. > This patch allows using other directories as rootpath. > > When you use genimage's mountpoints to generate an image with > multiple (non-empty) partitions, it does two things: > - copy $TARGET_DIR to $GENIMAGE_TMP/root > - move any mountpoint from GENIMAGE_TMP/root to $GENIMAGE_TMP/<mountpoint> > > If you want to have an additional partition besides the rootfs, you > will have to make sure it's content is in $TARGET_DIR so that genimage > can find it. > > If you also use rootfs generated by buildroot, you will end up > with two copies of the mountpoint: once in the rootfs itself, and once > in the partition using the mountpoint. Thanks for your contribution. While I understand the issue, I am not sure we want to extend support/scripts/genimage.sh for this. support/scripts/genimage.sh is meant to be a simple wrapper to genimage, for the simple cases. For the more complicated cases, your post-image script should call the genimage tool directly. support/scripts/genimage.sh was only added in Buildroot to avoid duplicating for our zillion defconfigs the same logic over and over again. I don't think it should be extended to cover more complicated cases: such complicated cases should call genimage directly. Arnout, Peter, any feedback on this ? I see that Raphael works at Essensium, so I would imagine that perhaps this contribution has been discussed with you Arnout before it was posted ? Or maybe not. Best regards, Thomas
On Wed, Sep 18, 2019 at 5:42 AM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Raphael, > > On Tue, 3 Sep 2019 14:09:51 +0200 > raphael.melotte@essensium.com wrote: > > > From: Raphaël Mélotte <raphael.melotte@essensium.com> > > > > Previously the rootpath was always set to $TARGET_DIR. > > This patch allows using other directories as rootpath. > > > > When you use genimage's mountpoints to generate an image with > > multiple (non-empty) partitions, it does two things: > > - copy $TARGET_DIR to $GENIMAGE_TMP/root > > - move any mountpoint from GENIMAGE_TMP/root to $GENIMAGE_TMP/<mountpoint> > > > > If you want to have an additional partition besides the rootfs, you > > will have to make sure it's content is in $TARGET_DIR so that genimage > > can find it. > > > > If you also use rootfs generated by buildroot, you will end up > > with two copies of the mountpoint: once in the rootfs itself, and once > > in the partition using the mountpoint. > > Thanks for your contribution. While I understand the issue, I am not > sure we want to extend support/scripts/genimage.sh for this. > support/scripts/genimage.sh is meant to be a simple wrapper to > genimage, for the simple cases. > > For the more complicated cases, your post-image script should call the > genimage tool directly. support/scripts/genimage.sh was only added in > Buildroot to avoid duplicating for our zillion defconfigs the same > logic over and over again. I don't think it should be extended to cover > more complicated cases: such complicated cases should call genimage > directly. > > Arnout, Peter, any feedback on this ? I see that Raphael works at > Essensium, so I would imagine that perhaps this contribution has been > discussed with you Arnout before it was posted ? Or maybe not. > > Best regards, > > Thomas Currently there are thirty board/*/post-image.sh files. Twenty of them are variants of genimage.sh and three of them call it so I believe that a more versatile genimage.sh would reduce some code duplication. Just my 2¢.
On Sat, 21 Sep 2019 21:55:19 -0300 Carlos Santos <unixmania@gmail.com> wrote: > Currently there are thirty board/*/post-image.sh files. Twenty of them > are variants of genimage.sh and three of them call it so I believe > that a more versatile genimage.sh would reduce some code duplication. I believe plenty of those post-image.sh predate the time we had genimage.sh and could simply be converted to use it, without changes to genimage.sh. For the remaining cases that need special things not provided by genimage.sh, I don't see the point of replicating all the options of the genimage tool in genimage.sh: in that case you'd better call genimage directly. It would just seem to be an extra layer that doesn't bring anything useful. As usual, this is just a personal opinion, and others can disagree, and I will not strongly oppose as what I'm exposing here is a weak feeling. Thomas
Hello Thomas, On 9/18/19 10:42 AM, Thomas Petazzoni wrote: > Thanks for your contribution. While I understand the issue, I am not > sure we want to extend support/scripts/genimage.sh for this. > support/scripts/genimage.sh is meant to be a simple wrapper to > genimage, for the simple cases. > > For the more complicated cases, your post-image script should call the > genimage tool directly. support/scripts/genimage.sh was only added in > Buildroot to avoid duplicating for our zillion defconfigs the same > logic over and over again. I don't think it should be extended to cover > more complicated cases: such complicated cases should call genimage > directly. > > Arnout, Peter, any feedback on this ? I see that Raphael works at > Essensium, so I would imagine that perhaps this contribution has been > discussed with you Arnout before it was posted ? Or maybe not. Thanks for your feedback :-) We indeed discussed it briefly together with Arnout, and one of the points was that the current value for the rootpath option does not make a lot of sense (but then, after thinking about it, choosing a better default value would probably be better than keeping the current one and making it customizable). The problem is that if you make any use of the current rootpath option by adding a (non-empty) partition, it will contain data that is already contained in the rootfs generated by Buildroot (if you choose to use it) which is a bit confusing for users. You could choose to not use the Buildroot-generated rootfs and use only genimage, but then you lack the fixes that are applied to the rootfs (changing ownership, setting special attributes, ...) and your rootfs will be unusable. Also, I agree with your point about keeping genimage.sh for the simple cases, but I don't think the cases where you would want to make use of rootpath are that complex: it could be for example that you want to use the default configuration for the board, but you just want to add a (non-empty) data partition next to the rootfs. However, if it didn't come up before, maybe it's not used as much as I would have thought... Kind regards, Raphaël
On 23/09/2019 09:44, Raphaël Mélotte wrote: > Hello Thomas, > > On 9/18/19 10:42 AM, Thomas Petazzoni wrote: >> Thanks for your contribution. While I understand the issue, I am not >> sure we want to extend support/scripts/genimage.sh for this. >> support/scripts/genimage.sh is meant to be a simple wrapper to >> genimage, for the simple cases. >> >> For the more complicated cases, your post-image script should call the >> genimage tool directly. support/scripts/genimage.sh was only added in >> Buildroot to avoid duplicating for our zillion defconfigs the same >> logic over and over again. I don't think it should be extended to cover >> more complicated cases: such complicated cases should call genimage >> directly. >> >> Arnout, Peter, any feedback on this ? I see that Raphael works at >> Essensium, so I would imagine that perhaps this contribution has been >> discussed with you Arnout before it was posted ? Or maybe not. > > Thanks for your feedback :-) > > We indeed discussed it briefly together with Arnout, and one of the points was > that the current value for the rootpath option does not make a lot of sense (but > then, after thinking about it, choosing a better default value would probably be > better than keeping the current one and making it customizable). Yes, we could do mkdir -p $(BINARIES_DIR)/rootpath ... --rootpath $(BINARIES_DIR)/rootpath which would allow a post-build script to install everything it needs there. > The problem is that if you make any use of the current rootpath option by adding > a (non-empty) partition, it will contain data that is already contained in the > rootfs generated by Buildroot (if you choose to use it) which is a bit confusing > for users. > > You could choose to not use the Buildroot-generated rootfs and use only > genimage, but then you lack the fixes that are applied to the rootfs (changing > ownership, setting special attributes, ...) and your rootfs will be unusable. Yes, the *proper* approach would be to add a genimage filesystem, where you supply a genimage.cfg file and it creates the required filesystems (so the ext4 is not created by Buildroot, but by genimage). > Also, I agree with your point about keeping genimage.sh for the simple cases, > but I don't think the cases where you would want to make use of rootpath are > that complex: it could be for example that you want to use the default > configuration for the board, but you just want to add a (non-empty) data > partition next to the rootfs. The idea for genimage.sh was really to avoid requiring a post-image script for each and every defconfig. It's not really supposed to be used outside of Buildroot. That's why it's in support/scripts and not in utils. Regards, Arnout > > However, if it didn't come up before, maybe it's not used as much as I would > have thought... > > > Kind regards, > > Raphaël >
Hello Raphaël, On Tue, 3 Sep 2019 14:09:51 +0200 raphael.melotte@essensium.com wrote: > From: Raphaël Mélotte <raphael.melotte@essensium.com> > > Previously the rootpath was always set to $TARGET_DIR. > This patch allows using other directories as rootpath. > > When you use genimage's mountpoints to generate an image with > multiple (non-empty) partitions, it does two things: > - copy $TARGET_DIR to $GENIMAGE_TMP/root > - move any mountpoint from GENIMAGE_TMP/root to $GENIMAGE_TMP/<mountpoint> The feedback on your patch was mostly negative. In addition, in the mean time, commit 31d1fb27b0e62a6542112a7476ff188f2f7b8d38 was made, which ensures --rootpath points to an empty directory, solving one of the issues pointed out by your patch. For other use-cases, a custom call to genimage is preferred, the support/scripts/genimage.sh is not meant to be a full-featured replacement to calling genimage directly. So I've marked your patch as Rejected in our patch tracking system. Best regards, Thomas
diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh index 039b3fef1d..071298a4b5 100755 --- a/support/scripts/genimage.sh +++ b/support/scripts/genimage.sh @@ -4,22 +4,26 @@ die() { cat <<EOF >&2 Error: $@ -Usage: ${0} -c GENIMAGE_CONFIG_FILE +Usage: ${0} -c GENIMAGE_CONFIG_FILE [--rootpath ROOTPATH] EOF exit 1 } # Parse arguments and put into argument list of the script -opts="$(getopt -n "${0##*/}" -o c: -- "$@")" || exit $? +opts="$(getopt -n "${0##*/}" -o c: -l rootpath: -- "$@")" || exit $? eval set -- "$opts" GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" +GENIMAGE_ROOTPATH="${TARGET_DIR}" while true ; do case "$1" in -c) GENIMAGE_CFG="${2}"; shift 2 ;; + --rootpath) + GENIMAGE_ROOTPATH="${2}" + shift 2 ;; --) # Discard all non-option parameters shift 1; break ;; @@ -33,8 +37,8 @@ done rm -rf "${GENIMAGE_TMP}" genimage \ - --rootpath "${TARGET_DIR}" \ - --tmppath "${GENIMAGE_TMP}" \ - --inputpath "${BINARIES_DIR}" \ - --outputpath "${BINARIES_DIR}" \ + --rootpath "${GENIMAGE_ROOTPATH}" \ + --tmppath "${GENIMAGE_TMP}" \ + --inputpath "${BINARIES_DIR}" \ + --outputpath "${BINARIES_DIR}" \ --config "${GENIMAGE_CFG}"