Bug#791724: RFS: w1retap/1.4.4-1 [ITP] -- Data logger for 1-Wire weather sensors
Hi Gianfranco,
Thanks again for another review and the suggestions you made! I have
made a number of the fixes you suggested and have tried to explain my
choices below. I have just uploaded a fresh version of the package to
http://mentors.debian.net/package/w1retap.
If you have any further comments they would really be appreciated,
alternatively if you think that the package is ready would you be
willing to sponsor the upload?
On 15 Oct 2016, at 11:10, Gianfranco Costamagna wrote:
> >The plugin packages contain .so files, the --noscripts option stops an
> >ldconfig run when installing. Without it I get a lintian useless
> >ldconfig run warnings.
>
> well, I usually don't care about such warnings, but I wonder if somewhen
> in the future the package starts shipping an external shared library
> and ldconfig won't be run because of this.
> (I'm not asking to change this, just wondering about a possible future
> side effect, and please note I have no clues about the possibilities
> of this scenario, and probably lintian will complain in such case)
>
I think w1retap is in maintenance mode, so this is currently unlikely.
However I'll keep it in mind in the future.
> >I did run all of the above and found some issues. Some of the issues
> >seemed to be more related to actual upstream development rather than
> >packaging. Most notably predicable file creation in /tmp, which is
> >fixed in the above mentioned movetmp.patch. Did you spot anything
> >else that needing fixing?
>
> mmm movetmp.patch...
>
> - w1->tmpname = "/tmp/.w1retap.dat";
> + w1->tmpname = "/var/lib/w1retap/.w1retap.dat";
>
> I don't think using /var/lib for tmp stuff is something
>
> this is a log, isn't /var/log something better?
> (with some logrotate stuff)
>
The /var/lib/w1retap directory contains the location for the default
w1retap data log file. If I were to record the data to PostgreSQL or
MySQL the actual data would end up somewhere else under /var/lib too.
Given that the data will be sensor readings (temperature, windspeed,
rainfall, etc) I would not expect this data to be rotated.
The ".w1retap.dat" file contains the last successful data reading. This
data can then be read by various other programs or scripts that only
want current readings (eg the current outside temperature). I think /tmp
is not the correct place to store it and I think that it does not fit
well into /var/log either. This is why I changed it to /var/lib.
> additional little points:
> - please enable hardening if possible, according to blhc
> http://debomatic-amd64.debian.net/distribution#unstable/w1retap/1.4.4-1/blhc
> somebody is overriding flags
>
> -
> DPKG_EXPORT_BUILDFLAGS = 1
> include /usr/share/dpkg/default.mk
>
>
> ^^ this is useless now
>
I removed the above and I think the buildflags or the include was
overriding the flags. The build now includes "-fPIE
-fstack-protector-strong -Wformat -Werror=format-security" when
building.
> - why the upstream build system is creating all this stuff in /usr/bin?
Dallas Semiconductor Corp (now Maxim Integrated) created the 1-wire
standard and devices. They created a software release (confusingly
called libusblinux300.tar.gz) to interface with the devices. The idea
was to enable anyone to make use of the devices quickly without
restrictions. It was this software that was used to create the w1retap
project.
The upstream build system continues to create the sample utilities that
were included in the original software release. While these utilities
may be useful for w1retap development, to my knowledge they are not
required to actually run a w1retap setup. This is why I've not packaged
those extra binary's.
> - doc might be split easily into a w1retap-doc package
>
Good idea, I've split it out and created w1retap-doc. This nicely
reduced the size of the main w1retap deb file. This also prompted me to
install another useful script called w1sensors.
> usr/lib/*/w1retap/libw1common.so*
> usr/lib/*/w1retap/libw1csv.so*
> usr/lib/*/w1retap/libw1file.so*
> usr/lib/*/w1retap/libw1serial.so*
> usr/lib/*/w1retap/libw1usb.so*
> usr/lib/*/w1retap/libw1xml.so*
>
> what about a single
> usr/lib/*/w1retap/lib*.so* entry?
>
I do like that shorter version, however that glob will match all the
available plugins. So it would also include mondo, mysql, odbc, etc.
For example libw1mongo.so.0 would be packaged in w1retap and
w1retap-mongo. I can't see a simple way to get around this.
> - why systemd as runtime dependency?
>
That looks like my mistake, I thought I needed to depend on it given
that I'm shipping a service file that starts the main daemon with
systemd. I have removed that dependency.
> - with compat 10 you can avoid --parallel and --with autoreconf
>
Excellent, I've changed to compat 10.
> this should be enough for now (and probably now the review is complete)
>
> last thing about your patch
>
> - w1->rcfile = strdup("/etc/default/w1retap");
> + w1->rcfile = strdup("/etc/w1retap.conf");
>
> but you also install etc/default/w1retap... why?
The files in /etc/default usually store settings and runtime parameters
for daemons. For example my setup passes "-t 60" as a parameter to
w1retap to make it take a reading every minute. I package a sample
/etc/default/w1retap with this information.
However the actual main w1retap configuration file is separate to this,
it is this file that specifies which sensors to read, where to write the
data and other such parameters. Unfortunately the default location for
this configuration file is also /etc/default/w1retap, hence why I made
the change to the file location.
Kind Regards
--
Tom
Reply to: