<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hello,</div><div>Before to send a new patch I have some questions:<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le mer. 17 juin 2020 à 22:38, Thomas Petazzoni <<a href="mailto:thomas.petazzoni@bootlin.com">thomas.petazzoni@bootlin.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, 16 Jun 2020 18:42:00 +0200<br>
mchalain <<a href="mailto:marc.chalain@gmail.com" target="_blank">marc.chalain@gmail.com</a>> wrote:<br>
<br>
> Gpiod is a little daemon to trig gpio event and launch scripts on level<br>
> changing events.<br>
> As udev or mdev, it reads rules files to attach scripts on events. and<br>
> launch the scripts with environment variables to describe the event.<br>
> It uses libgpiod to monitor the gpio and libconfig to read the rules.<br>
> It is tested on Raspberry Pi (0,3,4) with success, during few months.<br>
> <br>
> Signed-off-by: mchalain <<a href="mailto:marc.chalain@gmail.com" target="_blank">marc.chalain@gmail.com</a>><br>
<br>
Thanks a lot for your contribution. I have not reviewed it carefully.<br>
However, while it looks potentially interesting, I am personally always<br>
a bit concerned in packaging software components that look like<br>
"personal projects". This project has only been written by you, there<br>
are no contributions from others, no stars, no fork on Github, nothing<br>
indicates that anyone but you is using gpiod.<br>
<br></blockquote><div>This project comes from a professional project. I didn't find any tool</div><div>to launch an application on a switch event. I thought that is stupid to write</div><div>code in the application only for that, when there isn't time mandatory.</div><div>This is a personal project, but I hope to reuse it in the future.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
So I am wondering if it is not too early to have that in Buildroot. On<br>
the other hand, you could certainly say that having it in Buildroot<br>
would give it some exposure that might encourage some users to try it<br>
out.<br>
<br>
Anyway, we will need your Signed-off-by to use your full first name and<br>
last name, properly capitalized.<br>
<br>
<br>
> diff --git a/package/gpiod/Config.in b/package/gpiod/Config.in<br>
> new file mode 100644<br>
> index 0000000000..e9d5dc47f9<br>
> --- /dev/null<br>
> +++ b/package/gpiod/Config.in<br>
> @@ -0,0 +1,14 @@<br>
> +config BR2_PACKAGE_GPIOD<br>
> +     bool "gpiod: gpio monitor daemon"<br>
> +     depends on BR2_USE_MMU<br>
> +     depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8<br>
<br>
This needs a comment:<br>
<br>
        depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8 # libgpiod<br>
</blockquote><div>OK done <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     select BR2_PACKAGE_LIBGPIOD<br>
> +     select BR2_PACKAGE_LIBCONFIG<br>
> +     help<br>
> +       GPIOD monitors gpio events and start scripts.<br>
> +       The daemon loads rules defining a gpio and<br>
> +       the scripts to launch when the level of gpio changes.<br>
> +<br>
> +comment "gpiod: needs a toolchain w/ support of MMU and headers > 4.8"<br>
> +     depends on !BR2_USE_MMU<br>
> +     depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8<br>
<br>
No, this should be:<br>
<br>
comment "gpiod needs a toolchain w/ kernel headers >= 4.8"<br>
        depends on BR2_USE_MMU<br>
        depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8<br>
<br></blockquote><div>Are you sure for  "depends on BR2_USE_MMU" and not "depends on !BR2_USE_MMU" ?<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +RUNDIR=/var/run<br>
> +SBINDIR=/usr/sbin<br>
> +BINDIR=/usr/bin<br>
> +<br>
> +start() {<br>
> +     printf "Starting gpiod: "<br>
> +     start-stop-daemon -S -q --exec ${SBINDIR}/gpiod -- -D -p ${RUNDIR}/gpiod.pid<br>
> +     [ $? == 0 ] && echo "OK" || echo "FAILED"<br>
> +}<br>
> +stop() {<br>
> +     printf "Stopping gpiod: "<br>
> +     ${SBINDIR}/gpiod -K -p ${RUNDIR}/gpiod.pid<br>
> +     echo "OK"<br>
> +}<br>
<br>
Please follow the template package/busybox/S01syslogd for your init<br>
script.<br>
<br></blockquote><div>OK done <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> diff --git a/package/gpiod/gpiod.hash b/package/gpiod/gpiod.hash<br>
> new file mode 100644<br>
> index 0000000000..4a9c10297c<br>
> --- /dev/null<br>
> +++ b/package/gpiod/gpiod.hash<br>
> @@ -0,0 +1 @@<br>
> +sha256 f7c12fafcfb02515ae34d9502b4121d7980606fb53b57bee35143bd985bfdddc  gpiod-1.0.tar.gz<br>
<br>
We need a hash for the license file.<br>
<br></blockquote><div>OK done</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> diff --git a/package/gpiod/<a href="http://gpiod.mk" rel="noreferrer" target="_blank">gpiod.mk</a> b/package/gpiod/<a href="http://gpiod.mk" rel="noreferrer" target="_blank">gpiod.mk</a><br>
> new file mode 100644<br>
> index 0000000000..ed99f1d3be<br>
> --- /dev/null<br>
> +++ b/package/gpiod/<a href="http://gpiod.mk" rel="noreferrer" target="_blank">gpiod.mk</a><br>
> @@ -0,0 +1,52 @@<br>
> +################################################################################<br>
> +#<br>
> +# gpiod<br>
> +#<br>
> +################################################################################<br>
> +<br>
> +GPIOD_VERSION = 1.0<br>
> +GPIOD_SITE = $(call github,mchalain,gpiod,$(GPIOD_VERSION))<br>
<br>
We need GPIOD_LICENSE and GPIOD_LICENSE_FILES.<br>
<br></blockquote><div>OK done</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +GPIOD_MAKE_OPTS+=prefix=/usr<br>
> +GPIOD_MAKE_OPTS+=sysconfdir=/etc/gpiod<br>
<br>
Just one assignment, and unconditional:<br>
<br>
GPIOD_MAKE_OPTS = \<br>
        prefix=/usr \<br>
        sysconfdir=/etc/gpiod<br>
<br>
> +#GPIOD_MAKE_OPTS+=DEBUG=y<br>
<br>
Drop comments please.<br>
<br></blockquote><div>OK done <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +GPIOD_KCONFIG_FILE=$(GPIOD_PKGDIR)/gpiod_defconfig<br>
> +GPIOD_KCONFIG_EDITORS = config<br>
> +GPIOD_KCONFIG_OPTS = $(GPIOD_MAKE_OPTS)<br>
> +<br>
> +GPIOD_DEPENDENCIES += libgpiod<br>
> +GPIOD_DEPENDENCIES += libconfig<br>
<br>
One line for dependencies, and unconditional:<br>
<br>
GPIOD_DEPENDENCIES = libgpiod libconfig<br>
<br></blockquote><div>OK done <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +define GPIOD_LIBCONFIG_OPTS<br>
> +     $(call KCONFIG_ENABLE_OPT,LIBCONFIG,$(@D)/.config)<br>
> +endef<br>
> +<br>
> +define GPIOD_KCONFIG_FIXUP_CMDS<br>
> +     $(GPIOD_LIBCONFIG_OPTS)<br>
> +endef<br>
> +<br>
> +define GPIOD_BUILD_CMDS<br>
> +     $(TARGET_CONFIGURE_OPTS) $(TARGET_MAKE_ENV) \<br>
> +             $(MAKE1) -C $(@D) $(GPIOD_MAKE_OPTS)<br>
> +endef<br>
> +<br>
> +define GPIOD_INSTALL_TARGET_CMDS<br>
> +     $(INSTALL) -d -m 755 $(TARGET_DIR)/etc/gpiod/rules.d<br>
<br>
Why do you need this ?<br>
<br></blockquote><div>For the same reason as the gpiod_defconfig is empty. LIBCONFIG is the only option of gpiod.</div><div>In a future version, I would like to remove the dependencies to libconfig, and I prepared that here.</div><div>I will remove the configuration in the makefile, and add the entry in the configuration file.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     $(MAKE) -C $(@D) $(GPIOD_MAKE_OPTS) \<br>
> +             DESTDIR="$(TARGET_DIR)" DEVINSTALL=n install<br>
<br>
I don't think you need DEVINSTALL=n here<br>
<br></blockquote><div>Yes, it's right. This option disallows the headers files, but there aren't headers files.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +endef<br>
> +<br>
> +define GPIOD_INSTALL_INIT_SYSTEMD<br>
> +     $(INSTALL) -D -m 644 $(GPIOD_PKGDIR)/gpiod.service \<br>
> +             $(TARGET_DIR)/usr/lib/systemd/system/gpiod.service<br>
<br>
This is OK. Perhaps use $(@D) instead of $(GPIOD_PKGDIR)<br>
<br>
However, the service file was missing in your patch.<br>
<br></blockquote><div>OK done <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +     mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants<br>
> +     ln -fs ../../../../usr/lib/systemd/system/gpiod.service \<br>
> +             $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/gpiod.service<br>
<br>
These two commands are not needed, systemd preset-all is going to take<br>
care of this.<br>
<br></blockquote><div>OK thank, I remembered to need that, a few years ago.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +endef<br>
> +define GPIOD_INSTALL_INIT_SYSV<br>
> +     $(INSTALL) -D -m 755 $(GPIOD_PKGDIR)/S20gpiod \<br>
> +             $(TARGET_DIR)/etc/init.d/S20gpiod<br>
> +endef<br>
> +<br>
> +$(eval $(kconfig-package))<br>
<br>
I'm a bit skeptical on the usage of kconfig-package here. You are *not*<br>
using kconfig at all. You have a makefile that emulates some of the<br>
kconfig behavior, but you're not using kconfig. So if we later change<br>
how kconfig-package behave, to use some kconfig functionality that you<br>
do not emulate, things will no longer work for the gpiod package.<br>
<br></blockquote><div>Big question... I can use CONFIGURE_CMDS, but if the kconfig system from the</div><div>kernel changes, what will you do ?</div><div>The version compatibility of each build system is a debate in development community</div><div>for many years,</div><div>I remember of many discussions about the version of autotools and m4, the discussions</div><div>about the best usage between to load all makefiles only once or to run each one by one.</div><div>And I don't speak about the trolls about the best tools "autotools more stable </div><div>"scons easer with python" "cmake the greatest", some people add another wrapper <br></div><div>arount cmake.</div><div>I prefer to use only Makefile, it's proof, flexible and it does a good job. I'm sure that</div><div>in 10 or 20 years, some body will be able to write and correct a Makefile.</div><div>After I can write a CONFIGURE_CMDS to do the same as kconfig-package but</div><div>it's less flexible to change the options (yes the only one option of the application ;)</div><div>Tell me your preference.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> diff --git a/package/gpiod/gpiod_defconfig b/package/gpiod/gpiod_defconfig<br>
> new file mode 100644<br>
> index 0000000000..e69de29bb2<br>
<br>
So it's just empty ?<br>
<br>
Best regards,<br>
<br>
Thomas<br>
-- <br>
Thomas Petazzoni, CTO, Bootlin<br>
Embedded Linux and Kernel engineering<br>
<a href="https://bootlin.com" rel="noreferrer" target="_blank">https://bootlin.com</a><br>
</blockquote></div></div></div></div></div></div>