| Message ID | 1367757785-5850-1-git-send-email-plagnioj@jcrosoft.com |
|---|---|
| State | Superseded |
| Headers | show |
Jean-Christophe, All, See my comments inlined below. On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > add option to specify the configuration file > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > --- > package/ser2net/Config.in | 9 +++++++++ > package/ser2net/S50ser2net | 48 ++++++++++++++++++++++++++++++++++++++++++++ > package/ser2net/ser2net.mk | 27 +++++++++++++++++++++++++ > 3 files changed, 84 insertions(+) > create mode 100644 package/ser2net/S50ser2net > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in > index e3b8870..3480d85 100644 > --- a/package/ser2net/Config.in > +++ b/package/ser2net/Config.in > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET > > http://ser2net.sourceforge.net > > +if BR2_PACKAGE_SER2NET > + > +config BR2_PACKAGE_SER2NET_CONF > + string "Configuration file" > + help > + Configuration file. I think we should provide a default, basic configuration file that we install unconditionnally to the rootfs. The user will then be free to provide a post-build script to tweak that file. > diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk > index 378dc06..5a2539e 100644 > --- a/package/ser2net/ser2net.mk > +++ b/package/ser2net/ser2net.mk > @@ -9,4 +9,31 @@ SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net > SER2NET_LICENSE = GPLv2+ > SER2NET_LICENSE_FILES = COPYING > > +define SER2NET_INIT_SCRIPT_INSTALL > + $(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d > +endef > + > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL Please use SER2NET_INSTALL_INIT_SYSV to install that script, eg.: SER2NET_INSTALL_INIT_SYSV += SER2NET_INIT_SCRIPT_INSTALL this way, the init script will only be installed if a sysv init sequence is used (and not systemd). Also, would it be possible to provide a systemd unit? Ditto, use: SER2NET_INSTALL_INIT_SYSTEMD > + > +define SER2NET_INIT_SCRIPT_UNINSTALL > + rm -f $(TARGET_DIR)/etc/init.d/S50ser2net > +endef > + > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL I think we decided that uninstall targets are not needed, because uninstall is not working completely in Buildroot. Drop it. > + > +define SER2NET_CONF_INSTALL > + if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \ > + $(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \ > + $(TARGET_DIR)/etc/ser2net.conf ; \ > + fi > +endef > + > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL Always install a default configuration file, as said above. > +define SER2NET_CONFIG_UNINSTALL > + rm -f $(TARGET_DIR)/etc/ser2net.conf > +endef > + > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL Ditto for uninstall: drop it. Thank you! Regards, Yann E. MORIN.
On 15:50 Sun 05 May , Yann E. MORIN wrote: > Jean-Christophe, All, > > See my comments inlined below. > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > add option to specify the configuration file > > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > > --- > > package/ser2net/Config.in | 9 +++++++++ > > package/ser2net/S50ser2net | 48 ++++++++++++++++++++++++++++++++++++++++++++ > > package/ser2net/ser2net.mk | 27 +++++++++++++++++++++++++ > > 3 files changed, 84 insertions(+) > > create mode 100644 package/ser2net/S50ser2net > > > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in > > index e3b8870..3480d85 100644 > > --- a/package/ser2net/Config.in > > +++ b/package/ser2net/Config.in > > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET > > > > http://ser2net.sourceforge.net > > > > +if BR2_PACKAGE_SER2NET > > + > > +config BR2_PACKAGE_SER2NET_CONF > > + string "Configuration file" > > + help > > + Configuration file. > > I think we should provide a default, basic configuration file that we > install unconditionnally to the rootfs. The user will then be free to > provide a post-build script to tweak that file. I hate post-build it's not trackable and as it's completly board specific with x serial to export I do not want a generic file > > > > diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk > > index 378dc06..5a2539e 100644 > > --- a/package/ser2net/ser2net.mk > > +++ b/package/ser2net/ser2net.mk > > @@ -9,4 +9,31 @@ SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net > > SER2NET_LICENSE = GPLv2+ > > SER2NET_LICENSE_FILES = COPYING > > > > +define SER2NET_INIT_SCRIPT_INSTALL > > + $(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d > > +endef > > + > > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL > > Please use SER2NET_INSTALL_INIT_SYSV to install that script, eg.: > SER2NET_INSTALL_INIT_SYSV += SER2NET_INIT_SCRIPT_INSTALL > > this way, the init script will only be installed if a sysv init sequence > is used (and not systemd). > > Also, would it be possible to provide a systemd unit? > Ditto, use: SER2NET_INSTALL_INIT_SYSTEMD > > > + > > +define SER2NET_INIT_SCRIPT_UNINSTALL > > + rm -f $(TARGET_DIR)/etc/init.d/S50ser2net > > +endef > > + > > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL > > I think we decided that uninstall targets are not needed, because > uninstall is not working completely in Buildroot. Drop it. > > > + > > +define SER2NET_CONF_INSTALL > > + if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \ > > + $(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \ > > + $(TARGET_DIR)/etc/ser2net.conf ; \ > > + fi > > +endef > > + > > +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL > > Always install a default configuration file, as said above. > > > +define SER2NET_CONFIG_UNINSTALL > > + rm -f $(TARGET_DIR)/etc/ser2net.conf > > +endef > > + > > +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL > > Ditto for uninstall: drop it. > > Thank you! > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
Jean-Christophe, All, On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 15:50 Sun 05 May , Yann E. MORIN wrote: > > Jean-Christophe, All, > > > > See my comments inlined below. > > > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > add option to specify the configuration file > > > > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > > > --- > > > package/ser2net/Config.in | 9 +++++++++ > > > package/ser2net/S50ser2net | 48 ++++++++++++++++++++++++++++++++++++++++++++ > > > package/ser2net/ser2net.mk | 27 +++++++++++++++++++++++++ > > > 3 files changed, 84 insertions(+) > > > create mode 100644 package/ser2net/S50ser2net > > > > > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in > > > index e3b8870..3480d85 100644 > > > --- a/package/ser2net/Config.in > > > +++ b/package/ser2net/Config.in > > > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET > > > > > > http://ser2net.sourceforge.net > > > > > > +if BR2_PACKAGE_SER2NET > > > + > > > +config BR2_PACKAGE_SER2NET_CONF > > > + string "Configuration file" > > > + help > > > + Configuration file. > > > > I think we should provide a default, basic configuration file that we > > install unconditionnally to the rootfs. The user will then be free to > > provide a post-build script to tweak that file. > > I hate post-build it's not trackable and as it's completly board specific with > x serial to export I do not want a generic file It's not about what *you* want, it's about what is *best* for Buildroot and *all* its users, *and* is maintenable and easily reproduced (which pointing to a config file outside of the buildroot tree is definitely not) *and* can fit your use case (which what I suggest should). We're speaking about a _default_ and _simple_ configuration file. Of course it can not cover all use cases, but just exporting a single serial port would be enough. Customisation beyond that is really part of the post-build scripts. And if you think that post-build script are not trackable, then letting the user specify a config file (presumably out-side of the Buildroot tree) is no more trackable either. So this excuse does not hold water. And I *do* track my post-build and post-image scripts. Here's the setting I use: .../buildroot/ <- the buildroot tree .../board-X/ <- the board-X tree, which *is* a git tree .../board-X/configs/ <- dir with all .config files for board-X .../board-X/scripts/ <- dir with add scripts for board-X .../board-X/misc/ <- misc files to push into rootfs .../board-X/build/ <- untracked dir for out-of-tree build Then I have those files tracked in my git tree: .../board-X/configs/br.defconfig .../board-X/configs/linux.defconfig .../board-X/configs/bb.defconfig .../board-X/scripts/ssh-key-in-root.ssh.authorized_keys .../board-X/scripts/other-mount-points-in-etc.fstab .../board-X/scripts/create-sdcard-image .../board-X/scripts/checksum-images .../board-X/misc/S98dvb-devices [and so on...] Now, all I said above is my point of view. Just let see what the maintainer will accept. :-) Regards, Yann E. MORIN.
On 16:58 Sun 05 May , Yann E. MORIN wrote: > Jean-Christophe, All, > > On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 15:50 Sun 05 May , Yann E. MORIN wrote: > > > Jean-Christophe, All, > > > > > > See my comments inlined below. > > > > > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > add option to specify the configuration file > > > > > > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > > > > --- > > > > package/ser2net/Config.in | 9 +++++++++ > > > > package/ser2net/S50ser2net | 48 ++++++++++++++++++++++++++++++++++++++++++++ > > > > package/ser2net/ser2net.mk | 27 +++++++++++++++++++++++++ > > > > 3 files changed, 84 insertions(+) > > > > create mode 100644 package/ser2net/S50ser2net > > > > > > > > diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in > > > > index e3b8870..3480d85 100644 > > > > --- a/package/ser2net/Config.in > > > > +++ b/package/ser2net/Config.in > > > > @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET > > > > > > > > http://ser2net.sourceforge.net > > > > > > > > +if BR2_PACKAGE_SER2NET > > > > + > > > > +config BR2_PACKAGE_SER2NET_CONF > > > > + string "Configuration file" > > > > + help > > > > + Configuration file. > > > > > > I think we should provide a default, basic configuration file that we > > > install unconditionnally to the rootfs. The user will then be free to > > > provide a post-build script to tweak that file. > > > > I hate post-build it's not trackable and as it's completly board specific with > > x serial to export I do not want a generic file > > It's not about what *you* want, it's about what is *best* for Buildroot > and *all* its users, *and* is maintenable and easily reproduced (which > pointing to a config file outside of the buildroot tree is definitely > not) *and* can fit your use case (which what I suggest should). I want to put the ser2net.conf in the tree not our of tree > > We're speaking about a _default_ and _simple_ configuration file. Of > course it can not cover all use cases, but just exporting a single > serial port would be enough. Customisation beyond that is really part of > the post-build scripts. the problem here which port you export? the ttyS0? no it's mostly the default console how could you even think of any generic file as the name of the port(s) and configuration is board specific This will be completly board specific Best Regards, J.
Jean-Christophe, All, On Sun, May 05, 2013 at 10:19:32PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 16:58 Sun 05 May , Yann E. MORIN wrote: > > On Sun, May 05, 2013 at 04:32:52PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > On 15:50 Sun 05 May , Yann E. MORIN wrote: > > > > On Sun, May 05, 2013 at 02:43:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > > add option to specify the configuration file [--SNIP--] > > > > I think we should provide a default, basic configuration file that we > > > > install unconditionnally to the rootfs. The user will then be free to > > > > provide a post-build script to tweak that file. > > > > > > I hate post-build it's not trackable and as it's completly board specific with > > > x serial to export I do not want a generic file > > > > It's not about what *you* want, it's about what is *best* for Buildroot > > and *all* its users, *and* is maintenable and easily reproduced (which > > pointing to a config file outside of the buildroot tree is definitely > > not) *and* can fit your use case (which what I suggest should). > > I want to put the ser2net.conf in the tree not our of tree OK. So, why have it configurable if it is in the tree in the first place? > > We're speaking about a _default_ and _simple_ configuration file. Of > > course it can not cover all use cases, but just exporting a single > > serial port would be enough. Customisation beyond that is really part of > > the post-build scripts. > the problem here which port you export? > the ttyS0? no it's mostly the default console Of course, ttyS0 is special because it is the console. ttyS1 is a good candidate. > how could you even think of any generic file as the name of the port(s) and > configuration is board specific IIRC, I never said 'generic'; I said 'default and basic', which I agree did not completely convey what I meant. I think this file should be seen as a (kind of) example that would work on some boards (eg. a PC) but would anyway need further customisation. > This will be completly board specific Yes, I agree that will be board specific. However, providing a default configuration file that exports ttyS1 can serve as a basis for the user to customise. We can't expect everything in Buildroot to work without customisation. For example, a webserver will always have pages to serve, so the user will have to provide those. And provide a certificate if he wants to serve https. And so on... The same goes for this kind of configuration: provide a simple, basic config file that the user can further customise, or even entirely replace. And this kind of customisation is done with post-build scripts. However, an alternative solution would be to ask the user what port(s) to export, and generate the config file from ser2net.mk. If this is not enough, then a post-build script can be used to provide a more advanced config file. Regards, Yann E. MORIN.
diff --git a/package/ser2net/Config.in b/package/ser2net/Config.in index e3b8870..3480d85 100644 --- a/package/ser2net/Config.in +++ b/package/ser2net/Config.in @@ -8,5 +8,14 @@ config BR2_PACKAGE_SER2NET http://ser2net.sourceforge.net +if BR2_PACKAGE_SER2NET + +config BR2_PACKAGE_SER2NET_CONF + string "Configuration file" + help + Configuration file. + +endif + comment "ser2net requires a toolchain with IPV6 support" depends on !BR2_INET_IPV6 diff --git a/package/ser2net/S50ser2net b/package/ser2net/S50ser2net new file mode 100644 index 0000000..90b6af1 --- /dev/null +++ b/package/ser2net/S50ser2net @@ -0,0 +1,48 @@ +#!/bin/sh + +NAME=ser2net +DAEMON=/usr/sbin/${NAME} +CONFIG_FILE=/etc/${NAME}.conf +PIDFILE=/var/run/$NAME.pid +trap "" 1 +trap "" 15 +test -f $DAEMON || exit 0 + +start() { + echo -n "Starting Ser2Net: " + $DAEMON -c $CONFIG_FILE -P ${PIDFILE} + if [ $? != 0 ]; then + echo "FAILED" + exit 1 + else + echo "done" + fi +} + +stop() { + echo -n "Stopping Ser2Net: " + kill -9 `cat ${PIDFILE}` + echo "done" +} + +case "$1" in + start) + start + ;; + + stop) + stop + ;; + + restart) + stop + start + ;; + + *) + echo "Usage: /etc/init.d/S50ser2net {start|stop|restart}" + exit 1 + ;; +esac + +exit 0 diff --git a/package/ser2net/ser2net.mk b/package/ser2net/ser2net.mk index 378dc06..5a2539e 100644 --- a/package/ser2net/ser2net.mk +++ b/package/ser2net/ser2net.mk @@ -9,4 +9,31 @@ SER2NET_SITE = http://downloads.sourceforge.net/project/ser2net/ser2net SER2NET_LICENSE = GPLv2+ SER2NET_LICENSE_FILES = COPYING +define SER2NET_INIT_SCRIPT_INSTALL + $(INSTALL) -m 0755 package/ser2net/S50ser2net $(TARGET_DIR)/etc/init.d +endef + +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_INSTALL + +define SER2NET_INIT_SCRIPT_UNINSTALL + rm -f $(TARGET_DIR)/etc/init.d/S50ser2net +endef + +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_INIT_SCRIPT_UNINSTALL + +define SER2NET_CONF_INSTALL + if [ "$(BR2_PACKAGE_SER2NET_CONF)" != "" ] ; then \ + $(INSTALL) -m 0644 $(BR2_PACKAGE_SER2NET_CONF) \ + $(TARGET_DIR)/etc/ser2net.conf ; \ + fi +endef + +SER2NET_POST_INSTALL_TARGET_HOOKS += SER2NET_CONF_INSTALL + +define SER2NET_CONFIG_UNINSTALL + rm -f $(TARGET_DIR)/etc/ser2net.conf +endef + +SER2NET_POST_UNINSTALL_TARGET_HOOKS += SER2NET_CONFIG_UNINSTALL + $(eval $(autotools-package))
add option to specify the configuration file Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> --- package/ser2net/Config.in | 9 +++++++++ package/ser2net/S50ser2net | 48 ++++++++++++++++++++++++++++++++++++++++++++ package/ser2net/ser2net.mk | 27 +++++++++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 package/ser2net/S50ser2net