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