<div dir="ltr"><div class="gmail_extra">Hi Thomas, Yann, all,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Sorry for the large RTT, real life called, she wanted her lawn mower back... :)</div><div class="gmail_extra">Some additional comments on Inetutils v2 patch.</div><div class="gmail_extra">v3 shall follow shortly.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 5, 2015 at 9:49 AM, Károly Kasza <span dir="ltr"><<a href="mailto:kaszak@gmail.com" target="_blank">kaszak@gmail.com</a>></span> wrote:</div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> +++ b/package/inetutils/0001-PATH_PROCNET_DEV.patch<br>
<br>
</span>A lowercase file name would be nicer here.<br>
<div><div><br></div></div></blockquote><div><br></div></span><div>OK</div></div></div></div></blockquote><div><br></div><div>This is the exact (case-sensitive) name of the missing C preprocessor macro defined in the patch. I would rather not lower-case it.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> +config BR2_PACKAGE_INETUTILS_PING<br>
> +     bool "ping"<br>
> +     depends on BR2_INET_IPV6<br>
<br>
</span>ping6 needs IPv6. But why ping also needs IPv6 ?<br>
<span><br></span></blockquote><div><br></div></span><div>Yeah, this look ridiculous I know, but it did need IPv6 for the compilation as I remember. I'll double check. </div></div></div></div></blockquote><div><div><br></div><div>ping/ping_common.h has an include for netinet/icmp6.h</div><div>It would require some work to create a patch for ping not to depend on icmp6.h - a new separate ping_common6.h or something.</div><div>I think this should be left intact.</div><div><br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +config BR2_PACKAGE_INETUTILS_RCP<br>
> +     bool "rcp"<br>
> +     depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC<br>
<br>
</span>Isn't rcp like rsh and al. ? What is the relation with RPC support?<br>
<span><br></span></blockquote><div> </div></span><div>Like IPv6 above. It did need it at compilation time. I guess it's some dependency of an #include.</div></div></div></div></blockquote><div><br></div><div><div>Compilation fails with undefined reference to `rcmd', which is defined in "libc/inet/rpc/rcmd.c" in uClibc.</div><div>I'm sure this needs RPC support.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> > +config BR2_PACKAGE_INETUTILS_RLOGIN</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +     bool "rlogin"<br>
> +     depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC<br>
<br>
</span>Same here.<br>
<br>
If RPC is really needed, I guess we want to be able to use either the<br>
native RPC support of the toolchain, or libtirpc.<br></blockquote><div><br></div></span><div>OK, I'll check.</div></div></div></div></blockquote><div><br></div><div><div>I'm afraid that doesn't seem to be possible.</div><div>libtirpc does not include rcmd.c or an "int rcmd()" function which is called by rcp/rsh/rlogin implementations in inetutils.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> +config BR2_PACKAGE_INETUTILS_RLOGIND<br>
> +     bool "rlogind"<br>
> +     depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC<br>
<br>
</span>Also RPC support needed?<br></blockquote><div><br></div></span><div>:) all r* needed RPC for compilation.</div></div></div></div></blockquote><div><br></div><div><div>Except rexec. My bad.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> +INETUTILS_LICENSE = GPLv3<br>
<br>
</span>The license seems to be GPLv3+, not GPLv3.<br></blockquote><div><br></div></span><div>Dunno, I'll check.</div></div></div></div></blockquote><div><br></div><div><div>COPYING looks a pretty standard GPLv3 to me, but of course it states that any later version of GPL can be applied to it / which is also defined in the original GPLv3.</div><div>I think all GPLv3 is GPLv3+ in this sense? I don't know the difference.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +define INETUTILS_PERMISSIONS<br>
> +/bin/ping                    f 4755 0 0 - - - - -<br>
> +/bin/ping6                   f 4755 0 0 - - - - -<br>
> +/usr/bin/rlogin                      f 4755 0 0 - - - - -<br>
> +/usr/bin/rsh                 f 4755 0 0 - - - - -<br>
> +/usr/bin/rcp                 f 4755 0 0 - - - - -<br>
> +/usr/bin/traceroute          f 4755 0 0 - - - - -<br>
> +endef<br>
<br>
</span>This will not work, since some of the programs are optional, and the<br>
<pkg>_PERMISSIONS mechanism will bail out with an error if a file<br>
doesn't exist. So you have to do something like:<br>
<br>
ifeq ($(BR2_PACKAGE_INETUTILS_PING),y)<br>
INETUTILS_PERMISSIONS += /bin/ping f 4755 0 0 - - - - -$(sep)<br>
INETUTILS_CONF_OPTS += --enable-ping<br>
INETUTILS_USR_BINS_MOVE += ping<br>
endif<br>
<span><br></span></blockquote><div><br></div></span><div>OK</div></div></div></div></blockquote><div> </div><div>Done in v3.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +INETUTILS_CONF_OPTS += --libexecdir=/usr/sbin --disable-clients --disable-servers<br>
<br>
</span>Please add a comment that explains why we pass --libexecdir=/usr/sbin.<br></blockquote><div><br></div></span><div>OK</div></div></div></div></blockquote><div><br></div><div>(Because by default the daemons are installed in /usr/libexec, which would make it possible to have multiple versions of the same tools in different paths. Buildroot installs the daemons in the (Linux default) /usr/sbin. Inetutils should do the same.)</div><div>Commented in v3.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_BUSYBOX),busybox)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_NCURSES),ncurses)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_NET_TOOLS),net-tools)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_IPUTILS),iputils)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_RSH_REDONE),rsh-redone)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_SYSKLOGD),sysklogd)<br>
> +INETUTILS_DEPENDENCIES += $(if $(BR2_PACKAGE_WHOIS),whois)<br>
<br>
</span>I think we should split these dependencies in two categories with a<br>
comment:<br>
<br>
# ncurses is a dependency of some inetutils programs<br>
... ncurses stuff here ....<br>
<br>
# inetutils provides programs also provided by other packages, and we<br>
# want inetutils to win over those packages<br>
... busybox, net-tools, iputils, rsh-redone, sysklogd, whois ....<br></blockquote><div><br></div></span><div>OK</div><div><div><div></div></div></div></div></div></div></blockquote><div><br></div><div>Done in v3.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_DNSDOMAINNAME),--enable-dnsdomainname)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_FTP),--enable-ftp)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_HOSTNAME),--enable-hostname)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_IFCONFIG),--enable-ifconfig)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_LOGGER),--enable-logger)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_PING),--enable-ping)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_PING6),--enable-ping6)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_RCP),--enable-rcp)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_REXEC),--enable-rexec)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_RLOGIN),--enable-rlogin)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_RSH),--enable-rsh)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TALK),--enable-talk)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TELNET),--enable-telnet)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TFTP),--enable-tftp)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TRACEROUTE),--enable-traceroute)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_WHOIS),--enable-whois)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_FTPD),--enable-ftpd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_INETD),--enable-inetd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_REXECD),--enable-rexecd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_RLOGIND),--enable-rlogind)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_RSHD),--enable-rshd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_SYSLOGD),--enable-syslogd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TALKD),--enable-talkd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TELNETD),--enable-telnetd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_TFTPD),--enable-tftpd)<br>
> +INETUTILS_CONF_OPTS += $(if $(BR2_PACKAGE_INETUTILS_UUCPD),--enable-uucpd)<br>
> +<br>
> +# Move binaries to proper path (possibly overwriting other utility versions)<br>
> +define INETUTILS_MOVE_PROPER_PATH<br>
> +     if [ -e $(TARGET_DIR)/usr/bin/dnsdomainname ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/bin/dnsdomainname $(TARGET_DIR)/bin/dnsdomainname; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/bin/ping ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/bin/ping $(TARGET_DIR)/bin/ping; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/bin/ping6 ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/bin/ping6 $(TARGET_DIR)/bin/ping6; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/bin/hostname ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/bin/hostname $(TARGET_DIR)/bin/hostname; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/sbin/syslogd ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/sbin/syslogd $(TARGET_DIR)/sbin/syslogd; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/bin/ifconfig ]; then \<br>
> +             mv -f $(TARGET_DIR)/usr/bin/ifconfig $(TARGET_DIR)/sbin/ifconfig; \<br>
> +     fi;<br>
<br>
</div></div>Use the INETUTILS_USR_BINS_MOVE variable that will contain the list<br>
of programs to move from /usr/bin to /bin, and INETUTILS_USR_SBINS_MOVE<br>
will contain the list of programs to move from /usr/sbin to /sbin.<br></blockquote><div><br></div></div></div><div>OK</div></div></div></div></blockquote><div><br></div><div>Done in v3.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> +# Remove iputils and rsd-redone utilities to avoid redundancy<br>
> +define INETUTILS_REMOVE_REDUNDANT_TOOLS<br>
> +     if [ -e $(TARGET_DIR)/usr/sbin/in.rlogind ] && [ -e $(TARGET_DIR)/usr/sbin/rlogind ]; then \<br>
> +             rm $(TARGET_DIR)/usr/sbin/in.rlogind; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/sbin/in.rshd ] && [ -e $(TARGET_DIR)/usr/sbin/rshd ]; then \<br>
> +             rm $(TARGET_DIR)/usr/sbin/in.rshd; \<br>
> +     fi;<br>
> +     if [ -e $(TARGET_DIR)/usr/sbin/in.tftpd ] && [ -e $(TARGET_DIR)/usr/sbin/tftpd ]; then \<br>
> +             rm $(TARGET_DIR)/usr/sbin/in.tftpd; \<br>
> +     fi;<br>
<br>
</span>Try to also use a make variable to list the programs that should be<br>
removed. Maybe you should explicit a bit more why they are redundant.<br>
Redundant between what and what?<br></blockquote><div><br></div></span><div>These are the same tools from different packages with different filenames/paths.</div></div></div></div></blockquote><div><br></div><div>Added comment and variable in v3.</div><div><br></div><div>Best regards,</div><div>Karoly </div></div><br></div></div>