diff mbox series

[1/1] support/scripts/genimage.sh: allow setting rootpath from parameters.

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

Commit Message

Raphaël Mélotte Sept. 3, 2019, 12:09 p.m. UTC 
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.

To better illustrate that behavior, here's an example configuration:

image myhome.ext4 {
      name = "myhome"
      ext4{}
      size = 1G
      mountpoint = "/home/myhome"
      # Files will be copied from '<rootpath>/home/myhome'
}

image sdcard.img {
	hdimage {
		gpt = true
	}

	partition rootfs {
		partition-type = 0x83
		image = "rootfs.ext4"
		# This the buildroot-generated rootfs, which already
		# contains the mountpoint
	}

	partition myhome {
		partition-type = 0x83
		size = 1G
		image = "myhome.ext4"
		# genimage will generate and fill the partition itself
	}
}

As genimage correctly moves mountpoint from
<rootpath>/root/<mountpoint> to <rootpath>/<mountpoint>, a possible
solution is to not use the rootfs generated by buildroot, but to use
genimage to generate it from <rootpath>/root directly. The problem is
that buildroot apply some additional fixups in the rootfs generation,
that genimage doesn't have (see THIS_IS_NOT_YOUR_ROOT_FILESYSTEM).

The remaining solutions are then to either:
- Use the buildroot-generated rootfs, and patch genimage.sh
  to allow other rootpaths to be used.
- Generate a filesystem for genimage, so that it's called under
  fakeroot and have all the fixups.

This patch implements the first solution.

Signed-off-by: Raphaël Mélotte <raphael.melotte@essensium.com>
---
 support/scripts/genimage.sh | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Thomas Petazzoni Sept. 18, 2019, 8:42 a.m. UTC | #1 
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
Carlos Santos Sept. 22, 2019, 12:55 a.m. UTC | #2 
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¢.
Thomas Petazzoni Sept. 22, 2019, 7:01 a.m. UTC | #3 
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
Raphaël Mélotte Sept. 23, 2019, 7:44 a.m. UTC | #4 
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
Arnout Vandecappelle Sept. 23, 2019, 7:55 a.m. UTC | #5 
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
>
Thomas Petazzoni Jan. 6, 2020, 8:56 p.m. UTC | #6 
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 mbox series

Patch

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}"