| Message ID | 1460577820-32164-2-git-send-email-ezequiel@vanguardiasur.com.ar |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, On Wed, 13 Apr 2016 17:03:26 -0300, Ezequiel Garcia wrote: > diff --git a/fs/genimage/genimage.mk b/fs/genimage/genimage.mk > new file mode 100644 > index 000000000000..17c146b6b519 > --- /dev/null > +++ b/fs/genimage/genimage.mk > @@ -0,0 +1,21 @@ > +################################################################################ > +# > +# Generate a system image using genimage > +# > +################################################################################ > + > +ROOTFS_GENIMAGE_DEPENDENCIES = host-genimage > + > +GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" > + > +define ROOTFS_GENIMAGE_CMD > + $(RM) -rf ${GENIMAGE_TMP} && \ > + $(HOST_DIR)/usr/bin/genimage \ > + --rootpath ${TARGET_DIR} \ > + --tmppath ${GENIMAGE_TMP} \ > + --inputpath ${BINARIES_DIR} \ > + --outputpath ${BINARIES_DIR} \ > + --config ${BR2_TARGET_ROOTFS_GENIMAGE_CFG} > +endef > + > +$(eval $(call ROOTFS_TARGET,genimage)) A genimage.cfg file typically references some filesystem images like ext2/3/4 images. But I don't see anything in your implementation that ensures that the filesystem images that are used by the genimage.cfg file are produced *before* the genimage image is created. Am I missing something ? Also, I think the infra should probably handle the dosfstools/mtools dependency that we need for many platforms to build a small FAT filesystem. So maybe a sub-option like BR2_TARGET_ROOTFS_GENIMAGE_BUILDS_FATFS or something like that, that would just select the appropriate packages. Best regards, Thomas
On 04/13/16 22:03, Ezequiel Garcia wrote: > Currently, all the boards using genimage are using the same > command incantation. Therefore, let's introduce a new filesystem > infra to factorize them and allow easier genimage setup. > > This commit adds a new genimage infra, by calling genimage > with a user-provided config file. The user is still responsible > of enabling the apropriate rootfs filesystem images. And for enabling dosfstools/mtools (as observed by Thomas). > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > fs/Config.in | 1 + > fs/genimage/Config.in | 20 ++++++++++++++++++++ > fs/genimage/genimage.mk | 21 +++++++++++++++++++++ > 3 files changed, 42 insertions(+) > create mode 100644 fs/genimage/Config.in > create mode 100644 fs/genimage/genimage.mk > > diff --git a/fs/Config.in b/fs/Config.in > index 51ccf28169ef..94fe1446047f 100644 > --- a/fs/Config.in > +++ b/fs/Config.in > @@ -5,6 +5,7 @@ source "fs/cloop/Config.in" > source "fs/cpio/Config.in" > source "fs/cramfs/Config.in" > source "fs/ext2/Config.in" > +source "fs/genimage/Config.in" I think this shouldn't be done alphabetically, but really at the end. > source "fs/initramfs/Config.in" > source "fs/iso9660/Config.in" > source "fs/jffs2/Config.in" > diff --git a/fs/genimage/Config.in b/fs/genimage/Config.in > new file mode 100644 > index 000000000000..749494652464 > --- /dev/null > +++ b/fs/genimage/Config.in > @@ -0,0 +1,20 @@ > +config BR2_TARGET_ROOTFS_GENIMAGE > + bool "full system image using genimage" > + help > + Generate a full system image using the genimage tool > + and a user-provided configuration file. The image generated > + will contain a partition table and will be ready to be copied > + onto storage media, such as MMC, NAND or NOR devices. > + > + Keep in mind that you probably require to enable a rootfs > + filesystem image, according to the genimage config file that you > + provide. And to select mtools/dosfstools. Thomas proposed to add an option for that. For me, either an explicit genimage option or just adding the help text for it is OK. > + > +if BR2_TARGET_ROOTFS_GENIMAGE > + > +config BR2_TARGET_ROOTFS_GENIMAGE_CFG > + string "genimage config file" > + help > + Genimage user configuration file. > + > +endif # BR2_TARGET_ROOTFS_GENIMAGE > diff --git a/fs/genimage/genimage.mk b/fs/genimage/genimage.mk > new file mode 100644 > index 000000000000..17c146b6b519 > --- /dev/null > +++ b/fs/genimage/genimage.mk > @@ -0,0 +1,21 @@ > +################################################################################ > +# > +# Generate a system image using genimage > +# > +################################################################################ > + > +ROOTFS_GENIMAGE_DEPENDENCIES = host-genimage > + > +GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" Variable name should be ROOTFS_GENIMAGE_TMP > + > +define ROOTFS_GENIMAGE_CMD > + $(RM) -rf ${GENIMAGE_TMP} && \ && is not needed, treat it as two separate commands. -f is not needed, it is already in $(RM). All variables should use $(), not ${} (below as well). > + $(HOST_DIR)/usr/bin/genimage \ We don't generally align \ (annoying whitespace changes when you add a line). > + --rootpath ${TARGET_DIR} \ > + --tmppath ${GENIMAGE_TMP} \ > + --inputpath ${BINARIES_DIR} \ > + --outputpath ${BINARIES_DIR} \ > + --config ${BR2_TARGET_ROOTFS_GENIMAGE_CFG} > +endef > + > +$(eval $(call ROOTFS_TARGET,genimage)) I think that, like for initramfs, the infra has little to offer here. And it is in fact in the way, because (as observed by Thomas) you'll want to add rootfs-genimage: $(TARGETS_ROOTFS) which leads to a circular dependency. So to make sure that it happens in the right order, I think you need something like: rootfs-genimage: $(TARGETS_ROOTFS) $(ROOTFS_GENIMAGE_DEPENDENCIES) $(RM) -r $(ROOTFS_GENIMAGE_TMP) $(HOST_DIR)/usr/bin/genimage \ ... ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE),y) target-post-image: rootfs-genimage # Make sure the genimage dependencies appear in graph-depends show-targets: @echo $(ROOTFS_GENIMAGE_DEPENDENCIES) ifeq ($(wildcard $(BR2_TARGET_ROOTFS_GENIMAGE_CFG),)) $(error $(BR2_TARGET_ROOTFS_GENIMAGE_CFG) does not exist) endif However, I'm afraid that we're moving a bit too fast after all. There are several open issues still: - Do post-image scripts come before or after genimage? - What with the dosfstools/mtools dependency? - Should we support genimage.cfg files that are generated from a post-image script? - Should we support several genimage.cfg files, producing several images (e.g. a NAND and a SD image)? So, the current approach works well for the bundled defconfigs, but for real use cases I think it's a bit too limited to be practical after all. Therefore, at least as a first step, I guess it's better to just move the script to a common place, e.g. to package/genimage/post-image.sh. Let it take a single argument for the genimage.cfg file, and add some documentation to the manual. Regards, Arnout
Hello, On Wed, 13 Apr 2016 23:32:22 +0200, Arnout Vandecappelle wrote: > I think that, like for initramfs, the infra has little to offer here. And it > is in fact in the way, because (as observed by Thomas) you'll want to add > > rootfs-genimage: $(TARGETS_ROOTFS) > > which leads to a circular dependency. > > So to make sure that it happens in the right order, I think you need something > like: > > rootfs-genimage: $(TARGETS_ROOTFS) $(ROOTFS_GENIMAGE_DEPENDENCIES) > $(RM) -r $(ROOTFS_GENIMAGE_TMP) > $(HOST_DIR)/usr/bin/genimage \ > ... > > ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE),y) > > target-post-image: rootfs-genimage > > # Make sure the genimage dependencies appear in graph-depends > show-targets: > @echo $(ROOTFS_GENIMAGE_DEPENDENCIES) But then is it really something that belongs to fs/ ? It really isn't a filesystem. > However, I'm afraid that we're moving a bit too fast after all. There are > several open issues still: > > - Do post-image scripts come before or after genimage? > - What with the dosfstools/mtools dependency? > - Should we support genimage.cfg files that are generated from a post-image script? > - Should we support several genimage.cfg files, producing several images (e.g. a > NAND and a SD image)? > > So, the current approach works well for the bundled defconfigs, but for real > use cases I think it's a bit too limited to be practical after all. Do we need to support all real use cases? I think we should support the common use cases, and the more complicated use cases can be handled via a special post-image script. That's really the general philosophy of Buildroot IMO: handle the most common cases nicely, and leave enough extension scripts/hooks to allow people to plug their scripts to handle the more complicated/specific cases. Thanks, Thomas
Hello, On Wed, 13 Apr 2016 23:32:22 +0200, Arnout Vandecappelle wrote: > Therefore, at least as a first step, I guess it's better to just move the > script to a common place, e.g. to package/genimage/post-image.sh. Let it take a > single argument for the genimage.cfg file, and add some documentation to the manual. As an after-thought, this seems like a good idea. This is really a very simple and minimal change, and is easy to do. Thomas
On 13 April 2016 at 18:45, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Wed, 13 Apr 2016 23:32:22 +0200, Arnout Vandecappelle wrote: > >> Therefore, at least as a first step, I guess it's better to just move the >> script to a common place, e.g. to package/genimage/post-image.sh. Let it take a >> single argument for the genimage.cfg file, and add some documentation to the manual. > > As an after-thought, this seems like a good idea. This is really a very > simple and minimal change, and is easy to do. > I realize this is a simple step forward, but I'm wondering if we can sort out the open questions around a proper genimage infra, and just go for the big solution instead.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> # Make sure the genimage dependencies appear in graph-depends >> show-targets: >> @echo $(ROOTFS_GENIMAGE_DEPENDENCIES) > But then is it really something that belongs to fs/ ? It really isn't a > filesystem. No, it is closer to the post-image script. Where do you suggest to move it? system/? >> However, I'm afraid that we're moving a bit too fast after all. There are >> several open issues still: >> >> - Do post-image scripts come before or after genimage? >> - What with the dosfstools/mtools dependency? >> - Should we support genimage.cfg files that are generated from a post-image script? >> - Should we support several genimage.cfg files, producing several images (e.g. a >> NAND and a SD image)? >> >> So, the current approach works well for the bundled defconfigs, but for real >> use cases I think it's a bit too limited to be practical after all. > Do we need to support all real use cases? I think we should support the > common use cases, and the more complicated use cases can be handled via > a special post-image script. That's really the general philosophy of > Buildroot IMO: handle the most common cases nicely, and leave enough > extension scripts/hooks to allow people to plug their scripts to handle > the more complicated/specific cases. Agreed, but it is good to think about the questions Arnout listed to think about what is really the common use case. The definition of the post-image script was to run something at the very end, so I think we should do genimage before post-image (even though I could imagine use cases for the opposite as well). For the dosfstools/mtools dependencies I think a simple sub option pulling them in is most sensible. Supporting multiple genimage.cfg files (like we do for device_tables / post-build / post-image, ..) IMHO makes sense and looks simple to do.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > Hello, > On Wed, 13 Apr 2016 23:32:22 +0200, Arnout Vandecappelle wrote: >> Therefore, at least as a first step, I guess it's better to just move the >> script to a common place, e.g. to package/genimage/post-image.sh. Let it take a >> single argument for the genimage.cfg file, and add some documentation to the manual. > As an after-thought, this seems like a good idea. This is really a very > simple and minimal change, and is easy to do. The only complication is that you cannot directly pass arguments to post-image scripts, you have to use BR2_ROOTFS_POST_SCRIPT_ARGS (and it gets used for all scripts).
On 04/14/16 23:16, Peter Korsgaard wrote: >>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > > > Hello, > > On Wed, 13 Apr 2016 23:32:22 +0200, Arnout Vandecappelle wrote: > > >> Therefore, at least as a first step, I guess it's better to just move the > >> script to a common place, e.g. to package/genimage/post-image.sh. Let it take a > >> single argument for the genimage.cfg file, and add some documentation to the manual. > > > As an after-thought, this seems like a good idea. This is really a very > > simple and minimal change, and is easy to do. > > The only complication is that you cannot directly pass arguments to > post-image scripts, you have to use BR2_ROOTFS_POST_SCRIPT_ARGS (and it > gets used for all scripts). I really don't think that this is a limitation. Especially if you don't pass the file, but the directory as the argument; that would be potentially useful for other post-image scripts as well. So I tend to prefer the simple common-script-in-package/genimage solution. That said, if Ezequiel posts a patch that takes into account all my concerns, I'll be happy to Ack it. Regards, Arnout
On 04/14/16 10:33, Peter Korsgaard wrote: >>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > > Hi, > >>> # Make sure the genimage dependencies appear in graph-depends > >> show-targets: > >> @echo $(ROOTFS_GENIMAGE_DEPENDENCIES) > > > But then is it really something that belongs to fs/ ? It really isn't a > > filesystem. > > No, it is closer to the post-image script. Where do you suggest to move > it? system/? > > > >> However, I'm afraid that we're moving a bit too fast after all. There are > >> several open issues still: > >> > >> - Do post-image scripts come before or after genimage? > >> - What with the dosfstools/mtools dependency? > >> - Should we support genimage.cfg files that are generated from a post-image script? > >> - Should we support several genimage.cfg files, producing several images (e.g. a > >> NAND and a SD image)? > >> > >> So, the current approach works well for the bundled defconfigs, but for real > >> use cases I think it's a bit too limited to be practical after all. > > > Do we need to support all real use cases? I think we should support the > > common use cases, and the more complicated use cases can be handled via > > a special post-image script. That's really the general philosophy of > > Buildroot IMO: handle the most common cases nicely, and leave enough > > extension scripts/hooks to allow people to plug their scripts to handle > > the more complicated/specific cases. > > Agreed, but it is good to think about the questions Arnout listed to > think about what is really the common use case. > > The definition of the post-image script was to run something at the very > end, so I think we should do genimage before post-image (even though I > could imagine use cases for the opposite as well). Yes, so eventually we'll have a post-rootfs script... > > For the dosfstools/mtools dependencies I think a simple sub option > pulling them in is most sensible. > > Supporting multiple genimage.cfg files (like we do for device_tables / > post-build / post-image, ..) IMHO makes sense and looks simple to do. Well, compared to the really simple solution that Ezequiel has now, I think the code will look quite a bit more complicated when there are multiple genimage.cfg files. Oh, and one more question: should we somehow make sure that the images generated by the different scripts don't have the same name? Regards, Arnout
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, > On 04/14/16 10:33, Peter Korsgaard wrote: >> The definition of the post-image script was to run something at the very >> end, so I think we should do genimage before post-image (even though I >> could imagine use cases for the opposite as well). > Yes, so eventually we'll have a post-rootfs script... Possibly, yes ;) >> For the dosfstools/mtools dependencies I think a simple sub option >> pulling them in is most sensible. >> >> Supporting multiple genimage.cfg files (like we do for device_tables / >> post-build / post-image, ..) IMHO makes sense and looks simple to do. > Well, compared to the really simple solution that Ezequiel has now, I > think the code will look quite a bit more complicated when there are > multiple genimage.cfg files. Wouldn't it basically just sticking the logic in a loop over the words in BR2_TARGET_ROOTFS_GENIMAGE_CFG? > Oh, and one more question: should we somehow make sure that the > images generated by the different scripts don't have the same name? No, I don't think we should do any parsing/verification of the .cfg files. If the user specifies files that aren't built or overwrites already existing files then they are themselves to blame.
All, On 2016-04-14 23:37 +0200, Peter Korsgaard spake thusly: > >>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > > On 04/14/16 10:33, Peter Korsgaard wrote: > >> Supporting multiple genimage.cfg files (like we do for device_tables / > >> post-build / post-image, ..) IMHO makes sense and looks simple to do. > > > Well, compared to the really simple solution that Ezequiel has now, I > > think the code will look quite a bit more complicated when there are > > multiple genimage.cfg files. > > Wouldn't it basically just sticking the logic in a loop over the words > in BR2_TARGET_ROOTFS_GENIMAGE_CFG? It is already possible and perfectly legit to declare more than one image in a genimage config file, as far as I can see. Regards, Yann E. MORIN.
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: >> Wouldn't it basically just sticking the logic in a loop over the words >> in BR2_TARGET_ROOTFS_GENIMAGE_CFG? > It is already possible and perfectly legit to declare more than one > image in a genimage config file, as far as I can see. Ahh, so we can then just concatenate all files specified and pass to genimage - Nice!
diff --git a/fs/Config.in b/fs/Config.in index 51ccf28169ef..94fe1446047f 100644 --- a/fs/Config.in +++ b/fs/Config.in @@ -5,6 +5,7 @@ source "fs/cloop/Config.in" source "fs/cpio/Config.in" source "fs/cramfs/Config.in" source "fs/ext2/Config.in" +source "fs/genimage/Config.in" source "fs/initramfs/Config.in" source "fs/iso9660/Config.in" source "fs/jffs2/Config.in" diff --git a/fs/genimage/Config.in b/fs/genimage/Config.in new file mode 100644 index 000000000000..749494652464 --- /dev/null +++ b/fs/genimage/Config.in @@ -0,0 +1,20 @@ +config BR2_TARGET_ROOTFS_GENIMAGE + bool "full system image using genimage" + help + Generate a full system image using the genimage tool + and a user-provided configuration file. The image generated + will contain a partition table and will be ready to be copied + onto storage media, such as MMC, NAND or NOR devices. + + Keep in mind that you probably require to enable a rootfs + filesystem image, according to the genimage config file that you + provide. + +if BR2_TARGET_ROOTFS_GENIMAGE + +config BR2_TARGET_ROOTFS_GENIMAGE_CFG + string "genimage config file" + help + Genimage user configuration file. + +endif # BR2_TARGET_ROOTFS_GENIMAGE diff --git a/fs/genimage/genimage.mk b/fs/genimage/genimage.mk new file mode 100644 index 000000000000..17c146b6b519 --- /dev/null +++ b/fs/genimage/genimage.mk @@ -0,0 +1,21 @@ +################################################################################ +# +# Generate a system image using genimage +# +################################################################################ + +ROOTFS_GENIMAGE_DEPENDENCIES = host-genimage + +GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" + +define ROOTFS_GENIMAGE_CMD + $(RM) -rf ${GENIMAGE_TMP} && \ + $(HOST_DIR)/usr/bin/genimage \ + --rootpath ${TARGET_DIR} \ + --tmppath ${GENIMAGE_TMP} \ + --inputpath ${BINARIES_DIR} \ + --outputpath ${BINARIES_DIR} \ + --config ${BR2_TARGET_ROOTFS_GENIMAGE_CFG} +endef + +$(eval $(call ROOTFS_TARGET,genimage))
Currently, all the boards using genimage are using the same command incantation. Therefore, let's introduce a new filesystem infra to factorize them and allow easier genimage setup. This commit adds a new genimage infra, by calling genimage with a user-provided config file. The user is still responsible of enabling the apropriate rootfs filesystem images. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- fs/Config.in | 1 + fs/genimage/Config.in | 20 ++++++++++++++++++++ fs/genimage/genimage.mk | 21 +++++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 fs/genimage/Config.in create mode 100644 fs/genimage/genimage.mk