Preparing Poly/ML

[Lionel Elie Mamane]

(Switching that thread to
 debian-science-maintainers at lists.alioth.debian.org)

On Thu, Aug 14, 2008 at 10:25:29PM +0000, Achim D. Brucker wrote:

> I prepared a first package for Poly/ML

I had a look yesterday. If you feel strongly about any issue I raise,
feel free to argue with me; being your sponsor does not automatically
make me right :)

 - I saw that you took the pain to exterminate compiler warnings;
   in general, if you want to do that, it is not wrong per se, but in
   my opinion, it is a bit of a time waste. So don't feel like you
   have to.

   Also consider that every divergence from upstream has a non-zero
   cost; it makes it more work to package a new upstream version (need
   to port the patch to the new upstream version, conflict with other
   patches we may want to apply, ...). So it should be done only if
   there is an advantage to the patch. Frankly, the advantage to
   removing compiler warnings... Do you see any?

   Note that these problems just fade away if upstream takes the patch
   and applies it. In a nutshell, in general, exterminating compiler
   warnings is an upstream work, and not a packaging work. There is
   nothing wrong in doing upstream work from within Debian (we should
   not shy away from making the software we ship better), but

   1) don't feel forced to
   2) upstream work can be done within Debian, but should generally be
      sent back upstream!

   Another side of the issue is that if upstream work is necessary to
   get the package into good enough shape for Debian, then, well, you
   have to do it :)

 - Now, about the specific compiler warnings in compiling polyml:

   * "deprecated conversion from string constant to 'char*"' (as
     opposed to "const char*") and "passing argument N of function
     drops constant qualifier from XXXX"

     This kind of warnings, about things that are deprecated, but
     still work, are a perfect example of warnings that are not useful
     to fix within Debian only - it is useful to do it upstream, but
     doing it within Debian, the cost is not balanced by any actual
     advantage - yet; when it stops working, you need to do it. OK,
     some kind of weak future warning.

     These kind of warnings are also only worth fixing at all if you
     fix the root of the problem, not only put a drape in front of the
     problem. In my understanding, that is what you did with all those
     (char*) casts. All they do is shut gcc up. The root of the
     problem is that libtif2 does not use const qualifiers in its API
     for strings it does not change. The problem is there. Trying to
     "fix" it in every caller is just, in my opinion, insanity.

     (Adding const qualifiers in the struct type definitions is
      another story; that change looks fine, but send it upstream,
      else we have to maintain that change, for no gain. You can put
      it in Debian in the meantime - actually that's a good way to
      test it - but if upstream doesn't take the patch, drop it.)

     IMO, the best way to shut gcc up on this point is compiling with
     -Wno-write-strings, not adding casts everywhere.

   * warning: dereferencing type-punned pointer will break strict-aliasing rules

     That's actually a warning that bothers me quite a bit
     more. Warning about a real problem. To me, it suggests that it
     should compile with -fno-strict-aliasing. It seems someone tried
     to do that, because -fno-strict-aliasing is in CFLAGS, but not in
     CXXFLAGS.

 - Your changes also do some pure whitespace-damage (spaces that
   become tabs or vice-versa, removing or adding spaces at the end of
   a line, ...). That's usually a bad idea, because it creates
   artificial conflicts in merges later, pollutes diffs, etc.

 - The "arm" Debian port is dying, and being replaced by "armel". Put
   "armel" instead of "arm" in the Architecture field of the packages.

 - You commented and uncommented dh_* calls so many times I got
   confused. I need to look at that again after lunch.


That's all for now.

-- 
Lionel