| Message ID | 1354531204-17481-1-git-send-email-jeremy.rosen@openwide.fr |
|---|---|
| State | Superseded |
| Headers | show |
Dear Jérémy Rosen, On Mon, 3 Dec 2012 11:40:03 +0100, Jérémy Rosen wrote: > diff --git a/fs/live/live.mk b/fs/live/live.mk > new file mode 100644 > index 0000000..33fe515 > --- /dev/null > +++ b/fs/live/live.mk > @@ -0,0 +1,20 @@ > +############################################################# > +# > +# Build the live root filesystem directory > +# > +############################################################# > + > + > +define ROOTFS_LIVE_CMD > + sudo rsync -a --no-o --no-g --delete-during $(TARGET_DIR)/ $(BR2_TARGET_ROOTFS_LIVE_DEST)/ > +endef Do we really want to include sudo commands in Buildroot? I'm not sure we want. What about instead a post-image script hook, that users can use to extract automatically the tarball image somewhere? Thomas
> > + > > +define ROOTFS_LIVE_CMD > > + sudo rsync -a --no-o --no-g --delete-during $(TARGET_DIR)/ > > $(BR2_TARGET_ROOTFS_LIVE_DEST)/ > > +endef > > Do we really want to include sudo commands in Buildroot? I'm not sure > we want. that's the question I wanted answered by sending this patch early... I'm still quite new in this community and I am not sure how the buildroot philosophy is seen note that I check for buildroot properly in the PRE_GEN_HOOKS and that I don't try to install host-sudo (which is not possible anyway) I personally think that building target/ with sudo instead of fakeroot would be a stupid idea, but that's not what this patch does. This patch simply adds a new optional target to ease NFS deployment, > > What about instead a post-image script hook, that users can use to > extract automatically the tarball image somewhere? > yes, that would work too, I can do that locally to solve my particular problem, but I don't think you would accept it upstream either. If you consider that deploying the live filesystem is not buildroot's job, fine I'll probably do it with post-build scripts instead, but I personally believe that NFS is the most common target and that doing it in buildroot-core is more convinient and makes more sense pos-install scripts needs to be rewritten by each user, and doing it manually is a bit error prone (especially if you have lots of devs sharing a buildroot project through git, you can have weird side effects with people not cleaning before deplying the new FS) again, I personally think this use-case makes sense, especially in big teams where some members don't want to learn what NFS is (yeah, sounds stupid, but it does happen) and I don't think there are any major drawback (we test sudo properly, we only call it for a specific use-case where it is really needed) except the philosophical question of "should a build tool use a tool giving root-priv like sudo" which in the case of NFS doesn't change much since the user will have to use it himself (either directly or trough post-image script). I'd really think it makes sense in a "type make, reboot the target, test your change" philosophy which matches really well the idea behind NFS based developement, but it's your call as with any core change... Regards Jérémy > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com >
On 03/12/12 11:40, Jérémy Rosen wrote: > add a new target to deploy a live filesystem to be used with NFS or as a chroot > > Signed-off-by: Jérémy Rosen<jeremy.rosen@openwide.fr> > --- > v2 : implement Arnoult's suggestion, update manual entry > --- > docs/manual/beyond-buildroot.txt | 16 +++++++--------- > fs/Config.in | 1 + > fs/live/Config.in | 16 ++++++++++++++++ > fs/live/live.mk | 20 ++++++++++++++++++++ > 4 files changed, 44 insertions(+), 9 deletions(-) > create mode 100644 fs/live/Config.in > create mode 100644 fs/live/live.mk > > diff --git a/docs/manual/beyond-buildroot.txt b/docs/manual/beyond-buildroot.txt > index e7b902d..adf3e83 100644 > --- a/docs/manual/beyond-buildroot.txt > +++ b/docs/manual/beyond-buildroot.txt > @@ -9,17 +9,15 @@ Boot the generated images > NFS boot > ~~~~~~~~ > > -To achieve NFS-boot, enable _tar root filesystem_ in the _Filesystem > -images_ menu. > +To achieve NFS-boot, enable _live root filesystem_ in the _Filesystem > +images_ menu and select a _live image location_ to choose where the live > +filesystem will be deployed. you can use _$(BINARIES_DIR)_ to easily > +build in +/path/to/output_dir/+ +/path/to/output_dir/images/+ > > -After a complete build, just run the following commands to setup the > -NFS-root directory: > +You will be asked for a password during the build. This is needed to create > +device entries in the target filesystem > > -------------------- > -sudo tar -xavf /path/to/output_dir/rootfs.tar -C /path/to/nfs_root_dir > -------------------- > - > -Then, you can execute a NFS-boot from your target. > +You will need to add the target path to +/etc/exports+. You will need to add the _live image location_ to +/etc/exports+. > > Chroot > ------ > diff --git a/fs/Config.in b/fs/Config.in > index 94154ea..de2377e 100644 > --- a/fs/Config.in > +++ b/fs/Config.in > @@ -11,5 +11,6 @@ source "fs/cpio/Config.in" > source "fs/iso9660/Config.in" > source "fs/initramfs/Config.in" > source "fs/romfs/Config.in" > +source "fs/live/Config.in" > > endmenu > diff --git a/fs/live/Config.in b/fs/live/Config.in > new file mode 100644 > index 0000000..60d03a7 > --- /dev/null > +++ b/fs/live/Config.in > @@ -0,0 +1,16 @@ > +config BR2_TARGET_ROOTFS_LIVE > + bool "live root filesystem" > + help > + uses sudo to create a live image of the filesystem > + this is mainly useful if you want to use your filesystem > + over NFS or a chroot Help text: Create a live image of the root filesystem that is directly usable as over NFS or chroot. Leave an empty line between this and the next config. > +config BR2_TARGET_ROOTFS_LIVE_DEST > + string "live image location" > + depends on BR2_TARGET_ROOTFS_LIVE > + default "$(BINARIES_DIR)/live" > + help > + The directory where the image should be stored. > + this directory will be emptied and recreated, it is given this -> This > + relative to the buildroot output/images directory The second part of this statement is redundant (it's true for all paths in the config and is not mentioned anywhere else) > + > + Don't put empty lines at the end of the file (but do make sure it ends with a newline). > diff --git a/fs/live/live.mk b/fs/live/live.mk > new file mode 100644 > index 0000000..33fe515 > --- /dev/null > +++ b/fs/live/live.mk > @@ -0,0 +1,20 @@ > +############################################################# > +# > +# Build the live root filesystem directory > +# > +############################################################# > + > + > +define ROOTFS_LIVE_CMD > + sudo rsync -a --no-o --no-g --delete-during $(TARGET_DIR)/ $(BR2_TARGET_ROOTFS_LIVE_DEST)/ Start commands with a tab - that makes it clearer that it's a command. Split the long line. > +endef > + > +define ROOTFS_LIVE_INIT > + if [ -z $(shell which sudo) ] ; then echo "sudo seems to not be installed on the host system" ; false ; fi ; \ This really should be checked in support/dependencies/dependencies.sh. > + if [ ! -t 0 ] ; then echo "live filesystem must be generated interactively" ; false ; fi ; \ > + if [ ! -t 2 ] ; then echo "live filesystem must be generated interactively" ; false ; fi ; Don't concatenate commands with \ but just start each command on a new line starting with a tab. (Of course, when you split the condition over several lines, then you have to use \). This test is not actually correct, because sudo will use the controlling terminal (/dev/tty) to ask a password, i.e. it still works when input and output are redirected. It will however not work when there is no controlling terminal, e.g. when it's invoked directly from a window manager menu. In addition, if an askpass program is specified, sudo doesn't even need a controlling terminal. So maybe it's better to drop the check after all. The sudo command itself will just fail with 'sudo: no tty present and no askpass program specified', which is clear enough. (I realize I asked for this check in the first place - but only idiots never change their mind :-) Regards, Arnout > +endef > + > +ROOTFS_LIVE_PRE_GEN_HOOKS += ROOTFS_LIVE_INIT > + > +$(eval $(call ROOTFS_TARGET,live))
On 03/12/12 14:42, Jeremy Rosen wrote: > >>> + >>> +define ROOTFS_LIVE_CMD >>> + sudo rsync -a --no-o --no-g --delete-during $(TARGET_DIR)/ >>> $(BR2_TARGET_ROOTFS_LIVE_DEST)/ >>> +endef >> >> Do we really want to include sudo commands in Buildroot? I'm not sure >> we want. I don't see a fundamental problem with using sudo within buildroot, as long as it's not for things that can be fakerooted. [snip] >> >> What about instead a post-image script hook, that users can use to >> extract automatically the tarball image somewhere? >> > > yes, that would work too, I can do that locally to solve my particular problem, but I don't think you would accept it upstream either. If you consider that deploying the live filesystem is not buildroot's job, fine I'll probably do it with post-build scripts instead, but I personally believe that NFS is the most common target and that doing it in buildroot-core is more convinient and makes more sense > > pos-install scripts needs to be rewritten by each user, and doing it manually is a bit error prone (especially if you have lots of devs sharing a buildroot project through git, you can have weird side effects with people not cleaning before deplying the new FS) +1 to that. For me, buildroot should make it *easy* to: - get an initial system up and running (quickly); - develop for the target board; - offer a reproducible and shareable build system. We already do very well on all three counts, but we can always improve. In addition, the NFS stuff is a FAQ. And FAQs are bugs, really. Regards, Arnout > again, I personally think this use-case makes sense, especially in big teams where some members don't want to learn what NFS is (yeah, sounds stupid, but it does happen) and I don't think there are any major drawback (we test sudo properly, we only call it for a specific use-case where it is really needed) except the philosophical question of "should a build tool use a tool giving root-priv like sudo" which in the case of NFS doesn't change much since the user will have to use it himself (either directly or trough post-image script). > > I'd really think it makes sense in a "type make, reboot the target, test your change" philosophy which matches really well the idea behind NFS based developement, but it's your call as with any core change... > > Regards > Jérémy
V3 is on the way.... I skip items I agree with and applied > > > +config BR2_TARGET_ROOTFS_LIVE_DEST > > + string "live image location" > > + depends on BR2_TARGET_ROOTFS_LIVE > > + default "$(BINARIES_DIR)/live" > > + help > > + The directory where the image should be stored. > > + this directory will be emptied and recreated, it is given > > this -> This > > > + relative to the buildroot output/images directory > > The second part of this statement is redundant (it's true for all > paths in the config and is not mentioned anywhere else) > and the whole thing is false since that path is now absolute (which makes more sense for NFS deployement) unless you want me to make it relative again... I believe absolute (whith the possibility to use env vars to find the buildroot location) makes most sense but that might be worth discussing > > > +endef > > + > > +define ROOTFS_LIVE_INIT > > + if [ -z $(shell which sudo) ] ; then echo "sudo seems to not be > > installed on the host system" ; false ; fi ; \ > > This really should be checked in > support/dependencies/dependencies.sh. > I didn't know about that file, but it seems to be more about dependencies for buildroot core than dependencies for a particular config option... otoh it's the only place where we check for stuff installed on the host (not compiled for host, natively installed on host) maybe the cleanest way to do it would be to have a virtual target ROOTFS_DEPENDS=native-xxx that would just check that "which xxx" exists on the system... that's a different patch I could have a look at if people think it's a good idea (I am not good at makefile so I have no idea if it's one line of code or if i'm going into makefile hell here...) > > + if [ ! -t 0 ] ; then echo "live filesystem must be generated > > interactively" ; false ; fi ; \ > > + if [ ! -t 2 ] ; then echo "live filesystem must be generated > > interactively" ; false ; fi ; > > Don't concatenate commands with \ but just start each command on a > new > line starting with a tab. (Of course, when you split the condition > over several lines, then you have to use \). > ok > This test is not actually correct, because sudo will use the > controlling terminal (/dev/tty) to ask a password, i.e. it still > works when input and output are redirected. It will however not > work when there is no controlling terminal, e.g. when it's invoked > directly from a window manager menu. In addition, if an askpass > program is specified, sudo doesn't even need a controlling terminal. > > So maybe it's better to drop the check after all. The sudo command > itself will just fail with 'sudo: no tty present and no askpass > program specified', which is clear enough. > i'll test that > (I realize I asked for this check in the first place - but only > idiots never change their mind :-) fair enough, nobody gets the first idea on the first try I could also run sudo with "fail instead of asking for a password" option, and have people add the proper line in the sudo config file to allow untar without password, but i'm not sure if it's better or worse i'll probably wait for your answer before sending V3 Thx for the proofreading Regards Jérémy Rosen
On 05/12/12 10:28, Jeremy Rosen wrote: > V3 is on the way.... > > I skip items I agree with and applied > >> >>> +config BR2_TARGET_ROOTFS_LIVE_DEST >>> + string "live image location" >>> + depends on BR2_TARGET_ROOTFS_LIVE >>> + default "$(BINARIES_DIR)/live" >>> + help >>> + The directory where the image should be stored. >>> + this directory will be emptied and recreated, it is given >> >> this -> This >> >>> + relative to the buildroot output/images directory >> >> The second part of this statement is redundant (it's true for all >> paths in the config and is not mentioned anywhere else) >> > > and the whole thing is false since that path is now absolute (which makes more sense for NFS deployement) > > unless you want me to make it relative again... I believe absolute (whith the possibility to use env vars to find the buildroot location) makes most sense but that might be worth discussing Like most other paths in buildroot, it's relative to $(TOP_DIR) (i.e. the buildroot directory). >>> +endef >>> + >>> +define ROOTFS_LIVE_INIT >>> + if [ -z $(shell which sudo) ] ; then echo "sudo seems to not be >>> installed on the host system" ; false ; fi ; \ >> >> This really should be checked in >> support/dependencies/dependencies.sh. >> > > I didn't know about that file, but it seems to be more about dependencies for buildroot core than dependencies for a particular config option... I forgot to add: similar to the java dependencies for classpath. > > otoh it's the only place where we check for stuff installed on the host (not compiled for host, natively installed on host) > > maybe the cleanest way to do it would be to have a virtual target > > ROOTFS_DEPENDS=native-xxx > > that would just check that "which xxx" exists on the system... > > that's a different patch I could have a look at if people think it's a good idea (I am not good at makefile so I have no idea if it's one line of code or if i'm going into makefile hell here...) That's not how we work - all native dependencies are checked in support/dependencies. [snip] > I could also run sudo with "fail instead of asking for a password" option, and have people add the proper line in the sudo config file to allow untar without password, but i'm not sure if it's better or worse No, we don't want to force people to change their host config just so they can use buildroot. Regards, Arnout > > i'll probably wait for your answer before sending V3 > > Thx for the proofreading > > Regards > Jérémy Rosen >
> > unless you want me to make it relative again... I believe absolute > > (whith the possibility to use env vars to find the buildroot > > location) makes most sense but that might be worth discussing > > Like most other paths in buildroot, it's relative to $(TOP_DIR) > (i.e. the buildroot directory). > my bad, you're right about that, i'll correct that > > >>> +endef > >>> + > >>> +define ROOTFS_LIVE_INIT > >>> + if [ -z $(shell which sudo) ] ; then echo "sudo seems to not > >>> be > >>> installed on the host system" ; false ; fi ; \ > >> > >> This really should be checked in > >> support/dependencies/dependencies.sh. > >> > > > > I didn't know about that file, but it seems to be more about > > dependencies for buildroot core than dependencies for a particular > > config option... > > I forgot to add: similar to the java dependencies for classpath. > > I didn't find anything like that in buildroot... there are some config-dependant checks in dependencies.sh (checking is locale is installed) but nothing class path related am I missing something or should I do it in a way similar to locale ? Regards Jérémy Rosen
> > > > I forgot to add: similar to the java dependencies for classpath. > > > > > > I didn't find anything like that in buildroot... there are some > config-dependant checks in dependencies.sh (checking is locale is > installed) but nothing class path related > > am I missing something or should I do it in a way similar to locale ? > ok, again it's my fault, I hadn't rebased in the last couple of days, v3 should be ready now
diff --git a/docs/manual/beyond-buildroot.txt b/docs/manual/beyond-buildroot.txt index e7b902d..adf3e83 100644 --- a/docs/manual/beyond-buildroot.txt +++ b/docs/manual/beyond-buildroot.txt @@ -9,17 +9,15 @@ Boot the generated images NFS boot ~~~~~~~~ -To achieve NFS-boot, enable _tar root filesystem_ in the _Filesystem -images_ menu. +To achieve NFS-boot, enable _live root filesystem_ in the _Filesystem +images_ menu and select a _live image location_ to choose where the live +filesystem will be deployed. you can use _$(BINARIES_DIR)_ to easily +build in +/path/to/output_dir/+ -After a complete build, just run the following commands to setup the -NFS-root directory: +You will be asked for a password during the build. This is needed to create +device entries in the target filesystem -------------------- -sudo tar -xavf /path/to/output_dir/rootfs.tar -C /path/to/nfs_root_dir -------------------- - -Then, you can execute a NFS-boot from your target. +You will need to add the target path to +/etc/exports+. Chroot ------ diff --git a/fs/Config.in b/fs/Config.in index 94154ea..de2377e 100644 --- a/fs/Config.in +++ b/fs/Config.in @@ -11,5 +11,6 @@ source "fs/cpio/Config.in" source "fs/iso9660/Config.in" source "fs/initramfs/Config.in" source "fs/romfs/Config.in" +source "fs/live/Config.in" endmenu diff --git a/fs/live/Config.in b/fs/live/Config.in new file mode 100644 index 0000000..60d03a7 --- /dev/null +++ b/fs/live/Config.in @@ -0,0 +1,16 @@ +config BR2_TARGET_ROOTFS_LIVE + bool "live root filesystem" + help + uses sudo to create a live image of the filesystem + this is mainly useful if you want to use your filesystem + over NFS or a chroot +config BR2_TARGET_ROOTFS_LIVE_DEST + string "live image location" + depends on BR2_TARGET_ROOTFS_LIVE + default "$(BINARIES_DIR)/live" + help + The directory where the image should be stored. + this directory will be emptied and recreated, it is given + relative to the buildroot output/images directory + + diff --git a/fs/live/live.mk b/fs/live/live.mk new file mode 100644 index 0000000..33fe515 --- /dev/null +++ b/fs/live/live.mk @@ -0,0 +1,20 @@ +############################################################# +# +# Build the live root filesystem directory +# +############################################################# + + +define ROOTFS_LIVE_CMD + sudo rsync -a --no-o --no-g --delete-during $(TARGET_DIR)/ $(BR2_TARGET_ROOTFS_LIVE_DEST)/ +endef + +define ROOTFS_LIVE_INIT + if [ -z $(shell which sudo) ] ; then echo "sudo seems to not be installed on the host system" ; false ; fi ; \ + if [ ! -t 0 ] ; then echo "live filesystem must be generated interactively" ; false ; fi ; \ + if [ ! -t 2 ] ; then echo "live filesystem must be generated interactively" ; false ; fi ; +endef + +ROOTFS_LIVE_PRE_GEN_HOOKS += ROOTFS_LIVE_INIT + +$(eval $(call ROOTFS_TARGET,live))
add a new target to deploy a live filesystem to be used with NFS or as a chroot Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr> --- v2 : implement Arnoult's suggestion, update manual entry --- docs/manual/beyond-buildroot.txt | 16 +++++++--------- fs/Config.in | 1 + fs/live/Config.in | 16 ++++++++++++++++ fs/live/live.mk | 20 ++++++++++++++++++++ 4 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 fs/live/Config.in create mode 100644 fs/live/live.mk