Re: suggested fix
Nicholas Bamber, le Thu 21 Jun 2012 13:04:17 +0100, a écrit : > ++/* As per #678358, Hurd defines AF_LINK but does not > ++ * provide the functionality. When that bug is fixed > ++ * These three lines could be removed and #678375 closed. > ++ */ > ++#ifdef __GNU__ > ++#undef AF_LINK > ++#endif I'm generally against such kind of hacks, because we then have to remember where they had been introduced once the actual issue is resolved, and get yet another upload there instead of a mere buildd giveback. I understand that getting "undefined AF_LINK" error message is clearer to people, but here the maintainers of the package already understand what the matter is, so they don't actually need the clearer message. Btw, the Hurd itself doesn't actually define AF_LINK, it's just that AF_LINK was added to the generic glibc header instead of system-specific header. Same for mremap flags which were discussed earlier in another thread. Samuel -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20120621122212.gc16...@type.bordeaux.inria.fr
Re: suggested fix
On 21/06/12 11:14, Steven Chamberlain wrote:
> On 21/06/12 08:57, Nicholas Bamber wrote:
>> 3.) You seem to see it fit to willfully cause an FTBS on Hurd, to make a
>> point.
>
> To willfully allow an existing FTBFS on GNU/Hurd, to become a more
> explanatory FTBFS, which would someday go away and keep the intended
> functionality once the cause had been resolved in its build-deps.
>
>> So I have raised a bug: #678358 to address the underlying issue.
>
> Yes that was the nice thing to do here, thanks.
>
>> * use #if defined(AF_LINK) && !defined(__GNU__) in both places as that
>> is as close to a feature check as we can get
>
> I'm fine with that, as it would be consistent, and it addresses the most
> important point which was the original test for (k)FreeBSD being too
> specific.
>
> Regards,
Jamie,
I have uploaded an NMU with a 2-day delay. The final diff is attached.
Given such a long and protracted discussion I hope you appreciate the
fix and incorporate it into your code as soon as possible. But of course
if you have an issue with it you can still intervene.
diff -Nru pmacct-0.14.0/debian/changelog pmacct-0.14.0/debian/changelog
--- pmacct-0.14.0/debian/changelog 2012-05-24 08:11:21.0 +0100
+++ pmacct-0.14.0/debian/changelog 2012-06-21 12:39:45.0 +0100
@@ -1,3 +1,11 @@
+pmacct (0.14.0-1.1) unstable; urgency=low
+
+ * Non-maintainer upload.
+ * Added patch to handle AF_LINK on hurd and kfreebsd
+platforms (Closes: #675836)
+
+ -- Nicholas Bamber Thu, 21 Jun 2012 12:39:40 +0100
+
pmacct (0.14.0-1) unstable; urgency=low
* New upstream release
diff -Nru pmacct-0.14.0/debian/control pmacct-0.14.0/debian/control
--- pmacct-0.14.0/debian/control2012-02-10 02:33:51.0 +
+++ pmacct-0.14.0/debian/control2012-06-21 11:43:56.0 +0100
@@ -2,7 +2,7 @@
Section: net
Priority: optional
Maintainer: Jamie Wilkinson
-Build-Depends: debhelper (>= 7), zlib1g-dev, libpcap-dev, libpq-dev,
libmysqlclient-dev, libsqlite3-dev
+Build-Depends: debhelper (>= 7), zlib1g-dev, libpcap-dev, libpq-dev,
libmysqlclient-dev, libsqlite3-dev, kfreebsd-kernel-headers[kfreebsd-any]
Standards-Version: 3.8.3
Package: pmacct
diff -Nru pmacct-0.14.0/debian/patches/af_link.patch
pmacct-0.14.0/debian/patches/af_link.patch
--- pmacct-0.14.0/debian/patches/af_link.patch 1970-01-01 01:00:00.0
+0100
+++ pmacct-0.14.0/debian/patches/af_link.patch 2012-06-21 11:54:28.0
+0100
@@ -0,0 +1,32 @@
+Author: Nicholas Bamber
+Subject: Problems with the sockaddr_dl structure
+ The first issue is that on kfreebsd platforms,
+ where AF_LINK is legitimately defined, the definition
+ of sockaddr_dl is in net/if_dl.h which the upstream source
+ does not #include. The second issue is that Hurd also
+ defines AF_LINK but does not actually implement
+ that functionality - #678358. Bug #678375 was raised
+ to clear this up when it is sorted out in Hurd.
+Bug-Debian: http://bugs.debian.org/675836
+Last-Update: 2012-06-21
+--- a/src/isis/sockunion.c
b/src/isis/sockunion.c
+@@ -27,6 +27,18 @@
+ #include "prefix.h"
+ #include "sockunion.h"
+
++/* As per #678358, Hurd defines AF_LINK but does not
++ * provide the functionality. When that bug is fixed
++ * These three lines could be removed and #678375 closed.
++ */
++#ifdef __GNU__
++#undef AF_LINK
++#endif
++
++#ifdef AF_LINK
++#include
++#endif
++
+ const char *
+ inet_sutop (union sockunion *su, char *str)
+ {
diff -Nru pmacct-0.14.0/debian/patches/series
pmacct-0.14.0/debian/patches/series
--- pmacct-0.14.0/debian/patches/series 1970-01-01 01:00:00.0 +0100
+++ pmacct-0.14.0/debian/patches/series 2012-06-21 11:31:02.0 +0100
@@ -0,0 +1 @@
+af_link.patch
Re: suggested fix
On 21/06/12 08:57, Nicholas Bamber wrote: > 3.) You seem to see it fit to willfully cause an FTBS on Hurd, to make a > point. To willfully allow an existing FTBFS on GNU/Hurd, to become a more explanatory FTBFS, which would someday go away and keep the intended functionality once the cause had been resolved in its build-deps. > So I have raised a bug: #678358 to address the underlying issue. Yes that was the nice thing to do here, thanks. > * use #if defined(AF_LINK) && !defined(__GNU__) in both places as that > is as close to a feature check as we can get I'm fine with that, as it would be consistent, and it addresses the most important point which was the original test for (k)FreeBSD being too specific. Regards, -- Steven Chamberlain ste...@pyro.eu.org -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe2f3f5.1090...@pyro.eu.org
Re: suggested fix
On 20/06/12 22:50, Steven Chamberlain wrote: > You still didn't address that in your reply. > Steven, You seem to have three issues: 1.) feature based tests rather than platform based tests. I totally get the desirability of this. It means new OS 's or improvements to OS's get picked up automatically. My issue was that this does not work in *this* case. Furthermore I was ignorant of certain specifics - which is why I was widely circulating the NMU - and so improperly cautious. 2.) a proper audit trail and making Hurd accountable for the actual issue Again I totally get this. 3.) You seem to see it fit to willfully cause an FTBS on Hurd, to make a point. I don't get that. Any point you might make would be lost in all the other Hurd FTBS's. Surely it is much better to do something positive. So I have raised a bug: #678358 to address the underlying issue. I will clone this bug to a follow up bug blocked by #678358. I will check the source code of the other packages that I am aware might have this issue and do the same if I find anything. I will not delay any more with this NMU and my proposed patch will * use #if defined(AF_LINK) && !defined(__GNU__) in both places as that is as close to a feature check as we can get * Have a comment explaining the issues in both the code and the patch header. -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe2d3f2.3060...@periapt.co.uk
Re: suggested fix
On 20/06/12 22:09, Nicholas Bamber wrote: > On 20/06/12 22:04, Steven Chamberlain wrote: >> This debdiff doesn't address the main point of my original mail: >> sockaddr_dl and net/if_dl.h are not (k)FreeBSD-specific, so a test for >> FreeBSD || FreeBSD_kernel would not be appropriate. You still didn't address that in your reply. > As I understood it you wanted the build to fail on Hurd so everyone > would know there was an AF_LINK/sockaddr_dl bug on Hurd. If there is really an issue in GNU/Hurd, such as a missing header, then yes, I'd prefer that the build fails[1], rather than add a workaround (with whatever consequences) to this package (which someone would have to remember to remove at the appropriate time, to restore the intended functionality). As it happens, if a workaround for the current FTBFS is all that's needed, the attached diff would be able to do that very cleanly. [1] Just to make sure this isn't the cause of any confusion: FTBFS on GNU/Hurd is not a blocker for Wheezy, testing migration or transitions. Regards, -- Steven Chamberlain ste...@pyro.eu.org --- pmacct-0.14.0.orig/src/isis/sockunion.c 2012-03-28 18:46:09.0 +0100 +++ pmacct-0.14.0/src/isis/sockunion.c 2012-06-20 22:32:29.672205632 +0100 @@ -596,6 +596,7 @@ return NULL; } +#if 0 /* Print sockunion structure */ static void __attribute__ ((unused)) sockunion_print (union sockunion *su) @@ -634,6 +635,7 @@ break; } } +#endif #ifdef ENABLE_IPV6 static int
Re: suggested fix
On 20/06/12 22:04, Steven Chamberlain wrote: > On 20/06/12 15:59, Nicholas Bamber wrote: >> Based upon the feedback I have received (including #debian-hurd) I am >> attaching a new debdiff. > > This debdiff doesn't address the main point of my original mail: > sockaddr_dl and net/if_dl.h are not (k)FreeBSD-specific, so a test for > FreeBSD || FreeBSD_kernel would not be appropriate. It might "work" but > would only replace one portability issue with another. > > The new test for AF_LINK && !GNU looks even worse to me. Does GNU/Hurd > _really_ define AF_LINK and yet not provide a net/if_dl.h with a > definition for sockaddr_dl? > > Regards, Steven, As I understood it you wanted the build to fail on Hurd so everyone would know there was an AF_LINK/sockaddr_dl bug on Hurd. I am not convinced that that is the right tack to take. I discussed the more general AF_LINK/sockaddr_dl issue with Pino today, and he's going to follow up on. But for now I'm just trying to get this package to build in a sane - if not ideal - way. I'm still open to arguments of course especially from the maintainer. -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe23c03.2040...@periapt.co.uk
Re: suggested fix
On 20/06/12 15:59, Nicholas Bamber wrote: > Based upon the feedback I have received (including #debian-hurd) I am > attaching a new debdiff. This debdiff doesn't address the main point of my original mail: sockaddr_dl and net/if_dl.h are not (k)FreeBSD-specific, so a test for FreeBSD || FreeBSD_kernel would not be appropriate. It might "work" but would only replace one portability issue with another. The new test for AF_LINK && !GNU looks even worse to me. Does GNU/Hurd _really_ define AF_LINK and yet not provide a net/if_dl.h with a definition for sockaddr_dl? Regards, -- Steven Chamberlain ste...@pyro.eu.org -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe23ac7.6020...@pyro.eu.org
Re: suggested fix
Jamie,
Based upon the feedback I have received (including #debian-hurd) I am
attaching a new debdiff. Unless I get any more feedback I'll probably
upload it tomorrow with a 2-day delay.
diff -Nru pmacct-0.14.0/debian/changelog pmacct-0.14.0/debian/changelog
--- pmacct-0.14.0/debian/changelog 2012-05-24 08:11:21.0 +0100
+++ pmacct-0.14.0/debian/changelog 2012-06-20 14:29:08.0 +0100
@@ -1,3 +1,11 @@
+pmacct (0.14.0-1.1) unstable; urgency=low
+
+ * Non-maintainer upload.
+ * Added special include of net/if_dl.h on kfreebsd
+and disabled AF_LINK support on Hurd (Closes: #675836)
+
+ -- Nicholas Bamber Wed, 20 Jun 2012 11:15:42 +0100
+
pmacct (0.14.0-1) unstable; urgency=low
* New upstream release
diff -Nru pmacct-0.14.0/debian/control pmacct-0.14.0/debian/control
--- pmacct-0.14.0/debian/control2012-02-10 02:33:51.0 +
+++ pmacct-0.14.0/debian/control2012-06-20 10:37:48.0 +0100
@@ -2,7 +2,8 @@
Section: net
Priority: optional
Maintainer: Jamie Wilkinson
-Build-Depends: debhelper (>= 7), zlib1g-dev, libpcap-dev, libpq-dev,
libmysqlclient-dev, libsqlite3-dev
+Build-Depends: debhelper (>= 7), zlib1g-dev, libpcap-dev, libpq-dev,
libmysqlclient-dev, libsqlite3-dev,
+ kfreebsd-kernel-headers[kfreebsd-any]
Standards-Version: 3.8.3
Package: pmacct
diff -Nru pmacct-0.14.0/debian/patches/kfreebsd.patch
pmacct-0.14.0/debian/patches/kfreebsd.patch
--- pmacct-0.14.0/debian/patches/kfreebsd.patch 1970-01-01 01:00:00.0
+0100
+++ pmacct-0.14.0/debian/patches/kfreebsd.patch 2012-06-20 14:34:08.0
+0100
@@ -0,0 +1,26 @@
+Author: Nicholas Bamber
+Subject: FTBS on kfreebsd
+Last-Update: 2012-06-20
+Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=675836
+--- a/src/isis/sockunion.c
b/src/isis/sockunion.c
+@@ -27,6 +27,10 @@
+ #include "prefix.h"
+ #include "sockunion.h"
+
++#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
++#include
++#endif
++
+ const char *
+ inet_sutop (union sockunion *su, char *str)
+ {
+@@ -619,7 +623,7 @@
+ break;
+ #endif /* ENABLE_IPV6 */
+
+-#ifdef AF_LINK
++#if defined(AF_LINK) && !defined(__GNU__)
+ case AF_LINK:
+ {
+ struct sockaddr_dl *sdl;
diff -Nru pmacct-0.14.0/debian/patches/series
pmacct-0.14.0/debian/patches/series
--- pmacct-0.14.0/debian/patches/series 1970-01-01 01:00:00.0 +0100
+++ pmacct-0.14.0/debian/patches/series 2012-06-20 10:44:37.0 +0100
@@ -0,0 +1 @@
+kfreebsd.patch
Re: suggested fix
Hi Nicholas, On 20/06/12 12:53, Nicholas Bamber wrote: > Sorry I didn't notice the FTBS on hurd as I was concentrating on the > red. I guess I should have trusted the bug report title more. I only noticed on buildd.d.o that the failure was the same there. > However I am confused at what your are proposing. If you're not convinced, then the patch you have is fine. It would fix the immediate FTBFS on kfreebsd-* and this could be revisited later. I was only trying to kill two (or more) birds with one stone here, by accommodating GNU/Hurd, and keeping portability to some other future k*BSD port, and if the patch is forwarded upstream they might like it to fix this on other arches they support. > For a start I cannot > find a net/if_dl.h file on Hurd. I'm not sure... be warned that packages.d.o might not be indexing package contents for GNU/Hurd. (At least once before this has caught me out). For the Hurd, I thought, if the header inclusion test was AF_LINK: 1. if it supports sockaddr_dl, and has net/if_dl.h, it would be fixed 2. if it supports sockaddr_dl, but currently lacks net/if_dl.h, the new build error would make the problem clearer, and it could build in future without changes after they provide the missing net/if_dl.h 3. if it doesn't support sockaddr_dl, the AF_LINK test is wrong in _both_ places so it wouldn't be able to build anyway; we'd be no worse-off. > Secondly I am not clear if using > AF_LINK as a conditional is a good idea. Surely that would change the > code on Linux, which is surely not what we want to do. I was assuming that platforms without sockaddr_dl don't define AF_LINK. I don't see it in the Linux headers. And the FXR pages also showed a correlation between AF_LINK being defined on a platform, and the existence of a net/if_dl.h containing the definition of sockaddr_dl. Regards, -- Steven Chamberlain ste...@pyro.eu.org -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe1c16f.8090...@pyro.eu.org
Re: suggested fix
On 20/06/12 12:27, Steven Chamberlain wrote: > On 20/06/12 11:56, Nicholas Bamber wrote: >> I have a proposed fix as attached. It's built, signed and ready to go. >> If you have intentions to fix it yourself please reply and do so >> promptly. I'll run my fix past a few people for feedback but after that >> I'll upload with a 2-day delay. > > Thanks for this. It looks okay and seems it would fix the FTBFS. > > However... I'm wondering if a more generic test could be used instead of > FreeBSD || FreeBSD_kernel. Other BSD's would need this header too for > the sockaddr_dl definition[1]. And it looks like GNU/Hurd was failing > on this same code so it possibly has (or should have) the same. > > [1] http://fxr.watson.org/fxr/trackident?v=FREEBSD9;i=sockaddr_dl > [2] > https://buildd.debian.org/status/fetch.php?pkg=pmacct&arch=hurd-i386&ver=0.14.0-1&stamp=1339115492 > > What about using AF_LINK as a test of whether to include this header? > It looks to me like it would work. > > This way we'd be helping out GNU/Hurd at the same time, the fix would be > more appropriate for upstream and it helps with future portability. > > Thanks again, > Regards, Steven, Sorry I didn't notice the FTBS on hurd as I was concentrating on the red. I guess I should have trusted the bug report title more. However I am confused at what your are proposing. For a start I cannot find a net/if_dl.h file on Hurd. Secondly I am not clear if using AF_LINK as a conditional is a good idea. Surely that would change the code on Linux, which is surely not what we want to do. Also googling for Hurd and sockaddr_dl has not so far turned up anything useful. What bugs such as #636510 and #256669 do suggest is that AF_LINK is a bad indicator of the presence of sockaddr_dl. Maybe the solution is to furher guard the #ifdef AF_LINK bits by requiring that the OS not be Hurd. -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4fe1b9cf.3020...@periapt.co.uk
