Code review comment for ~sergiodj/ubuntu/+source/openipmi:openipmi-merge-2.0.29-0.1

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

LGTM you were able to drop most of the things since they are upstream and/or in Debian by now which is a great result of our delta submission \o/.

The one remaining delta is from before me touching the package, I didn't look too much at it last time - but now that it is the only thing left we maybe should look more deeply.

It is reported to Debian as already as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=474087 (long ago).
It is reported as Fixed there in version openipmi/2.0.16-1.1

So might this be able to become a sync now?

The Ubuntu fix says the fix is "Forward-ported from 2.10.16-1.1, as it seems to have been dropped in Debian".
Indeed at upstream it was fixed (late) see https://sourceforge.net/p/openipmi/feature-requests/5/

This upstream fix is in since 2.0.22-1 and the reason it was dropped from Debian.

And indeed this LGTM now for cflags:
root@d10-sid:~# pkg-config --libs OpenIPMIpthread
-lOpenIPMIutils -lOpenIPMIpthread -lOpenIPMIutils -lOpenIPMI
root@d10-sid:~# pkg-config --cflags OpenIPMIpthread
-pthread

The one reason against it is that you usually want it on cflags AND ldflags and not just one of them. But at least the upstream bug reporter was happy with the change as is.

So this should be:
a) not needed - good to become a sync now as it was the only delta.
OR
b) our remaining fix of adding it to "Libs:" as well is indeed important.
   In that case almost +1 to this MP, but then please provide a follow on bug report
   upstream and explain it there. Link tat bug report from our patch header then.

What do you think on (a) vs (b) @Sergio?

review: Needs Fixing

« Back to merge proposal