Autoconf, libpq and replacement function

Magnus Hagander wrote:

How do I make this work with the autoconf magic? I see there is an
AC_CHECK_FNMATCH and AC_REPLACE_FNMATCH and so, but I have a feeling I
need to do something different since it's libpq?

AC_*_FNMATCH will figure out whether you need fnmatch(), so something
involving those is necessary.

For libpq, check libpq's Makefile for, say, snprintf, to get an idea
about the "something different".

Altogether, this might not be a trivial case.

Peter Eisentraut wrote:

Magnus Hagander wrote:

How do I make this work with the autoconf magic? I see there is an
AC_CHECK_FNMATCH and AC_REPLACE_FNMATCH and so, but I have a feeling I
need to do something different since it's libpq?

AC_*_FNMATCH will figure out whether you need fnmatch(), so something
involving those is necessary.

For libpq, check libpq's Makefile for, say, snprintf, to get an idea
about the "something different".

Altogether, this might not be a trivial case.

Hmm. If I did it the right way, it actually didn't seem too hard once I
found a good example. Attached - does that seem reasonable?

(will add msvc code as well)

//Magnus

Magnus Hagander wrote:

Peter Eisentraut wrote:

Magnus Hagander wrote:

How do I make this work with the autoconf magic? I see there is an
AC_CHECK_FNMATCH and AC_REPLACE_FNMATCH and so, but I have a feeling I
need to do something different since it's libpq?

AC_*_FNMATCH will figure out whether you need fnmatch(), so something
involving those is necessary.

For libpq, check libpq's Makefile for, say, snprintf, to get an idea
about the "something different".

Altogether, this might not be a trivial case.

Hmm. If I did it the right way, it actually didn't seem too hard once I
found a good example. Attached - does that seem reasonable?

(will add msvc code as well)

That was easy. I also reversed the accidentally-reversed #ifdef in port.h.

//Mangus

Magnus Hagander <magnus@hagander.net> writes:

*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 393,398 **** extern void unsetenv(const char *name);
--- 393,409 ----
extern void srandom(unsigned int seed);
#endif

+ #ifdef HAVE_FNMATCH
+ extern int fnmatch(const char *, const char *, int);
+ #define FNM_NOMATCH		1		/* Match failed. */
+ #define FNM_NOSYS		2		/* Function not implemented. */
+ #define FNM_NOESCAPE	0x01	/* Disable backslash escaping. */
+ #define FNM_PATHNAME	0x02	/* Slash must be matched by slash. */
+ #define FNM_PERIOD		0x04	/* Period must be matched by period. */
+ #define FNM_CASEFOLD	0x08	/* Pattern is matched case-insensitive */
+ #define FNM_LEADING_DIR	0x10	/* Ignore /<tail> after Imatch. */
+ #endif
+ 

Surely we must *not* be providing our own definitions of these symbols
when using a system version of fnmatch.

Also, judging from the comments in the autoconf manual, you'd better
use AC_FUNC_FNMATCH not just test whether the function exists.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 393,398 **** extern void unsetenv(const char *name);
--- 393,409 ----
extern void srandom(unsigned int seed);
#endif

+ #ifdef HAVE_FNMATCH
+ extern int fnmatch(const char *, const char *, int);
+ #define FNM_NOMATCH		1		/* Match failed. */
+ #define FNM_NOSYS		2		/* Function not implemented. */
+ #define FNM_NOESCAPE	0x01	/* Disable backslash escaping. */
+ #define FNM_PATHNAME	0x02	/* Slash must be matched by slash. */
+ #define FNM_PERIOD		0x04	/* Period must be matched by period. */
+ #define FNM_CASEFOLD	0x08	/* Pattern is matched case-insensitive */
+ #define FNM_LEADING_DIR	0x10	/* Ignore /<tail> after Imatch. */
+ #endif
+ 

Surely we must *not* be providing our own definitions of these symbols
when using a system version of fnmatch.

That's the define that I reversed in the second patch I sent. It's
supposed to be ifndef.

Also, judging from the comments in the autoconf manual, you'd better
use AC_FUNC_FNMATCH not just test whether the function exists.

Ok, will look at switching to that.

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Surely we must *not* be providing our own definitions of these symbols
when using a system version of fnmatch.

That's the define that I reversed in the second patch I sent. It's
supposed to be ifndef.

Okay.

Also, judging from the comments in the autoconf manual, you'd better
use AC_FUNC_FNMATCH not just test whether the function exists.

Ok, will look at switching to that.

Hmm ... actually there's still possibly an issue there: what if the
system provides a broken version of fnmatch? AC_FUNC_FNMATCH will not
set HAVE_FNMATCH, and then we might end up with #define conflicts
anyway.

Since fnmatch and the #define's are supposed to be provided by
<fnmatch.h>, I think you should probably put the substitute definitions
in a substitute fnmatch.h, not port.h, to avoid that risk.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Also, judging from the comments in the autoconf manual, you'd better
use AC_FUNC_FNMATCH not just test whether the function exists.

Ok, will look at switching to that.

Hmm ... actually there's still possibly an issue there: what if the
system provides a broken version of fnmatch? AC_FUNC_FNMATCH will not
set HAVE_FNMATCH, and then we might end up with #define conflicts
anyway.

Since fnmatch and the #define's are supposed to be provided by
<fnmatch.h>, I think you should probably put the substitute definitions
in a substitute fnmatch.h, not port.h, to avoid that risk.

Do we have an example where we do that before? I assume there is some
autoconfy way to make that include file only "appear" in the include
path if the system one doesn't exist or is broken?

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Since fnmatch and the #define's are supposed to be provided by
<fnmatch.h>, I think you should probably put the substitute definitions
in a substitute fnmatch.h, not port.h, to avoid that risk.

Do we have an example where we do that before? I assume there is some
autoconfy way to make that include file only "appear" in the include
path if the system one doesn't exist or is broken?

Not really. I'd suggest making the callers do something like

#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#else
#include "port/pg_fnmatch.h"
#endif

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Since fnmatch and the #define's are supposed to be provided by
<fnmatch.h>, I think you should probably put the substitute definitions
in a substitute fnmatch.h, not port.h, to avoid that risk.

Do we have an example where we do that before? I assume there is some
autoconfy way to make that include file only "appear" in the include
path if the system one doesn't exist or is broken?

Not really. I'd suggest making the callers do something like

#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#else
#include "port/pg_fnmatch.h"
#endif

How's that actually different from the
#ifdef HAVE_FNMATCH
#include <fnmatch.h> <-- happens in fe-secure.c
#else
#define .... <-- happens in port.h
#endif

If HAVE_FNMATCH isn't set, we still have the same problem, no?

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Not really. I'd suggest making the callers do something like

#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#else
#include "port/pg_fnmatch.h"
#endif

How's that actually different from the
#ifdef HAVE_FNMATCH
#include <fnmatch.h> <-- happens in fe-secure.c
#else
#define .... <-- happens in port.h
#endif

What's bothering me is that port.h gets included *everywhere*, and
might perhaps conflict with some indirect or accidental inclusion
of <fnmatch.h>.

It would also allow someone to forget the
#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#endif
part and have it still work, if they were testing on a broken platform.
It's better that both inclusions appear together instead of having the
alternative code paths effectively appear in two unrelated files.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Not really. I'd suggest making the callers do something like

#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#else
#include "port/pg_fnmatch.h"
#endif

How's that actually different from the
#ifdef HAVE_FNMATCH
#include <fnmatch.h> <-- happens in fe-secure.c
#else
#define .... <-- happens in port.h
#endif

What's bothering me is that port.h gets included *everywhere*, and
might perhaps conflict with some indirect or accidental inclusion
of <fnmatch.h>.

It would also allow someone to forget the
#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#endif
part and have it still work, if they were testing on a broken platform.
It's better that both inclusions appear together instead of having the
alternative code paths effectively appear in two unrelated files.

Ok, I see your argument now.

AFAICS, we're not doing this for any other functions though - or am I
too tired and just looking in the wrong place? Or is that because
they're just function definitions and not #defines?

(I want to be sure to stick whatever new file there is in the same place..)

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

AFAICS, we're not doing this for any other functions though - or am I
too tired and just looking in the wrong place? Or is that because
they're just function definitions and not #defines?
(I want to be sure to stick whatever new file there is in the same place..)

src/include/rusagestub.h is doing things more or less this way.

Also, right now we have got

src/include/getaddrinfo.h
src/include/getopt_long.h

which really make me itch now that I am contemplating the possibility
that we try to use libc-supplied code for these functions along with
our own header definitions.

I think the reason we've avoided putting such stuff into include/port/
is that right now that directory consists exclusively of OS-specific
files. Maybe we need another include subdirectory?

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

AFAICS, we're not doing this for any other functions though - or am I
too tired and just looking in the wrong place? Or is that because
they're just function definitions and not #defines?
(I want to be sure to stick whatever new file there is in the same place..)

src/include/rusagestub.h is doing things more or less this way.

Also, right now we have got

src/include/getaddrinfo.h
src/include/getopt_long.h

which really make me itch now that I am contemplating the possibility
that we try to use libc-supplied code for these functions along with
our own header definitions.

I think the reason we've avoided putting such stuff into include/port/
is that right now that directory consists exclusively of OS-specific
files. Maybe we need another include subdirectory?

Or in the same directly, called something else. Like fnmatchstub.h. See
attached, seems reasonable?

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Or in the same directly, called something else. Like fnmatchstub.h. See
attached, seems reasonable?

Seems okay to me. Note: in future, don't bother posting configure
diffs; nobody wants to look at that, just the changes in the autoconf
input files.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Or in the same directly, called something else. Like fnmatchstub.h. See
attached, seems reasonable?

Seems okay to me. Note: in future, don't bother posting configure
diffs; nobody wants to look at that, just the changes in the autoconf
input files.

Ok, will exclude it next time.

FYI, the if check in the configure (and configure.in) was backwards :-)
I posted the patch without testing it to get the "general idea" across.

Will run a set of test of the latest one on msvc as well, and then commit.

//magnus

If you want people to stop posting configure diffs you should remove
the damn configure file from CVS where it doesn't belong.

greg

On 21 Nov 2008, at 06:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Greg Stark <greg.stark@enterprisedb.com> writes:

If you want people to stop posting configure diffs you should remove
the damn configure file from CVS where it doesn't belong.

Well, that'd require everyone using CVS to have autoconf installed
--- and not just any autoconf, but the specific version.  Seems like
more pain than it's worth considering most people have little need to
fool with the configure script.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Or in the same directly, called something else. Like fnmatchstub.h. See
attached, seems reasonable?

Seems okay to me. Note: in future, don't bother posting configure
diffs; nobody wants to look at that, just the changes in the autoconf
input files.

Yea, it makes us ill, especially when it is the first diff in the patch. ;-)

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:

I'd suggest making the callers do something like

#ifdef HAVE_FNMATCH
#include <fnmatch.h>
#else
#include "port/pg_fnmatch.h"
#endif

The way Autoconf suggests to organize this is to provide a fake
fnmatch.h (they call it fnmatch_.h) and link it to fnmatch.h if it is
needed.

This way, callers don't need to know the difference.