On Tue, Apr 14, 2015 at 08:43:42PM -0700, Ben Pfaff wrote:
>
> [Guru: please see final paragraph]
>
> On Tue, Apr 07, 2015 at 03:54:30PM +0800, Kevin Lo wrote:
> > On FreeBSD using AC_SEARCH_LIBS is not sufficient for finding the
> > linker flags for linking with a pthread library.
> > Add macro ax_pthread.m4 from GNU autoconf macro archive and use
> > macro AX_PTHREAD.
> >
> > Signed-off-by: Kevin Lo <kevlo at FreeBSD.org>
>
> Thanks for the patch. I apologize for the delay in the review; I've
> been away on vacation for a week.
>
> I agree that using this macro will generally be a portability
> improvement.
>
> I think that the patch to configure.ac should add $PTHREAD_CFLAGS to
> OVS_CFLAGS instead of to CFLAGS, so that the user does not accidentally
> override it on the configure command line. For the rationale, please
> see the mention of CFLAGS in INSTALL.md and
> https://www.gnu.org/software/autoconf/manual/automake.html#Flag-Variables-Ordering
> in the Autoconf manual.
Ah, you're right. Thank you for taking the time to review my patch.
> I am also concerned that it this will break the Windows build.
> Currently Windows uses some Windows-only code in m4/openvswitch.m4,
> which requires Win32 builders to specify --with-pthread=<dir> on the
> configure command line. Ideally, we would want Windows builds to work
> the same as other builds. Maybe that would mean that Windows builders
> should specify the PTHREAD_* variables on the configure command line,
> instead of --with-pthread, or that the OVS pthread-win32 support should
> move from OVS_CHECK_WIN32 to somewhere around the new invocation of
> AX_PTHREAD. Either way, I think that this will require some change to
> what this patch does (and possibly an update to INSTALL.Windows.md)
> before it can go in. I'm CCing Guru, who knows the Windows build, to
> get his opinion.
Understood. Thanks for your clear explanation.
> Thanks,
>
> Ben.
Kevin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
