[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#791724: RFS: w1retap/1.4.4-1 [ITP] -- Data logger for 1-Wire weather sensors



Hi,

>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)




No, it's a odbc plugin and the mysql plugin is separate. The confusion was
caused by a typo on my part. In anycase, both are now fixed.


>> patches: remove-data-time-macro.patch
>> you can dpkg-parsechangelog and export the changelog date an build date.
>> (this will make the package reproducible I think)
>
>The current upstream code (src/w1retap.c) uses the "__DATE__" and
>"__TIME__" GCC standard macros directly. I don't think
>dpkg-parsechangelog can affect these macros. My patch replaces these
>macros with __BUILD_DATE__.


this seems really nice and upstreamable
>The .so files are not useful or used and outside the w1retap, they are
>all internal. Thus I agree and have removed all the libw1retap*
>packages. They act more like plugins so I now have a main w1retap
>package and 5 separate plugin package for each database type.


seems better now
>There are a number of patches that I will send upstream:
>- add-etc-w1retap-conf.patch
>- movetmp.patch
>- remove-data-time-macro.patch 
>- udev rule
>- systemd service


wonderful
>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)

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

- why the upstream build system is creating all this stuff in /usr/bin?
- doc might be split easily into a w1retap-doc package

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?

- why systemd as runtime dependency?
- with compat 10 you can avoid --parallel and --with autoreconf

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?

G.


Reply to: