Bug#791724: RFS: w1retap/1.4.4-1 [ITP] -- Data logger for 1-Wire weather sensors
Hi Gianfranco,
Thank you for the comments and suggestions, they are really appreciated.
Apologies for the delay, I have slowly worked through your email on and
off over the past few months. I think I have addressed most of the
issues you mentioned (see my comments below), so I have uploaded a fresh
version to http://mentors.debian.net/package/w1retap.
Thanks again for reviewing the package, I would appreciate any other
comments you have or uploading it if you feel it's ready.
On 01 Apr 2016, at 20:52, Gianfranco Costamagna wrote:
> control:
> "retrieved data" <-- double space
> std version is 3.9.7 now
> please run wrap-and-sort
> please remove quilt from b-d
> changelog:
> one single changelog entry please
>
> -rm -f $(CURDIR)/debian/w1retap.1 $(CURDIR)/debian/w1find.1 -> debian/clean might be easier to maintain
>
> rules:
> dh_auto_install is a no-op please remove
All fixed.
> dh_makeshlibs --noscripts <-- why?
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.
> remove quilt
Fixed.
> libw1retap0-odbc.install <-- is that the mysql wrapper?
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.
> service file:
> ExecStop=/bin/kill $MAINPID
>
> really needed?
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__.
> question:
> usually libraries are ship in this way
>
> libfooSONAME shipping libfoo.so.SONAME
>
> libfoo-dev with an hard dependency on libfooSONAME
> and a libfoo.so symlink to libfoo.so.SONAME and /usr/include/foo
> headers files.
>
> In your package you are shipping them together
>
> not an issue, but do you know what you are doing here?
> are them useful and being linked outside the package?
> are them just internal?
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.
> lots of stuff (systemd script udev rules) should be upstreamed to me
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
> check-all-the-things review:
> codespell --quiet-level=3
> cppcheck -j1 --quiet -f .
> grep -riE 'fixme|todo|hack|xxx' .
> grep -r '/tmp/' .
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?
Regards
--
Tom
Reply to: