Re: [PATCHES] utils C files

I have applied the following diff to make the sharing of C files among
modules more sane. Instead of having configure.in set the file name to
strdup.o and have the Makefiles specify the path, I set it to the full
path $(top_builddir)/src/utils/strdup.o and have the makefiles use
that directly, rather than going through with 'make -c dirname
filename'.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian writes:

> I have applied the following diff to make the sharing of C files among
> modules more sane. Instead of having configure.in set the file name to
> strdup.o and have the Makefiles specify the path, I set it to the full
> path $(top_builddir)/src/utils/strdup.o and have the makefiles use
> that directly, rather than going through with 'make -c dirname
> filename'.

Don't do that, it doesn't work. Building outside the source tree, weird
compilers, etc. The current state was the result of much labor to get rid
of exactly the state you reintroduced.

A secondary objection is that I've been meaning to replace configure
checks of the form

AC_CHECK_FUNCS(inet_aton, [], INET_ATON='inet_aton.o')
AC_SUBST(INET_ATON)

with one integrated macro, which doesn't work if we have to encode the
path into configure.

Also, do not tab-indent comments in makefiles. That makes them part of
the command to execute.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> Bruce Momjian writes:
>
> > I have applied the following diff to make the sharing of C files among
> > modules more sane. Instead of having configure.in set the file name to
> > strdup.o and have the Makefiles specify the path, I set it to the full
> > path $(top_builddir)/src/utils/strdup.o and have the makefiles use
> > that directly, rather than going through with 'make -c dirname
> > filename'.
>
> Don't do that, it doesn't work. Building outside the source tree, weird
> compilers, etc. The current state was the result of much labor to get rid
> of exactly the state you reintroduced.

Well, the actual problem was that there was inconsistency in the way
things where handled, e.g. some had their own rules for making the *.o
files if the *.o files were out of the current directory, other didn't.
I can change it but it has to be consistent. What do you suggest?

> A secondary objection is that I've been meaning to replace configure
> checks of the form
>
> AC_CHECK_FUNCS(inet_aton, [], INET_ATON='inet_aton.o')
> AC_SUBST(INET_ATON)
>
> with one integrated macro, which doesn't work if we have to encode the
> path into configure.

The path is only the thing we assign to the variable. I can't see how
that effects the configure script. Actually, once we move stuff into
the same directory, it will not matter. They will all be in the same
directory so you can just prepend whatever directory we need.

> Also, do not tab-indent comments in makefiles. That makes them part of
> the command to execute.

Fixed. Thanks.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian writes:

> Well, the actual problem was that there was inconsistency in the way
> things where handled, e.g. some had their own rules for making the *.o
> files if the *.o files were out of the current directory, other didn't.
> I can change it but it has to be consistent. What do you suggest?

Can you point to one example of such an inconsistency? I can't find one.

The rule of thumb is that rules involving the C compiler should only use
files in the current directory. Otherwise you don't know where those
files are going to end up.

> > A secondary objection is that I've been meaning to replace configure
> > checks of the form
> >
> > AC_CHECK_FUNCS(inet_aton, [], INET_ATON='inet_aton.o')
> > AC_SUBST(INET_ATON)
> >
> > with one integrated macro, which doesn't work if we have to encode the
> > path into configure.
>
> The path is only the thing we assign to the variable. I can't see how
> that effects the configure script.

What I want this to read in the end is

PGAC_SOME_MACRO([func1 func2 func3])

> Actually, once we move stuff into the same directory, it will not
> matter.

True, that will help a lot.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> Bruce Momjian writes:
>
> > Well, the actual problem was that there was inconsistency in the way
> > things where handled, e.g. some had their own rules for making the *.o
> > files if the *.o files were out of the current directory, other didn't.
> > I can change it but it has to be consistent. What do you suggest?
>
> Can you point to one example of such an inconsistency? I can't find one.

Sure, interfaces/libpq had:

OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \
pqexpbuffer.o dllist.o md5.o pqsignal.o fe-secure.o \
$(INET_ATON) $(SNPRINTF) $(STRERROR)

while psql/Makefile had what I think you wanted:

OBJS=command.o common.o help.o input.o stringutils.o mainloop.o \
copy.o startup.o prompt.o variables.o large_obj.o print.o describe.o \
sprompt.o tab-complete.o mbprint.o

all: submake psql

ifdef STRDUP
OBJS+=$(top_builddir)/src/utils/strdup.o

$(top_builddir)/src/utils/strdup.o:
$(MAKE) -C $(top_builddir)/src/utils strdup.o
endif

ifdef STRERROR
OBJS+=$(top_builddir)/src/backend/port/strerror.o

$(top_builddir)/src/backend/port/strerror.o:
$(MAKE) -C $(top_builddir)/src/backend/port strerror.o
endif

ifdef SNPRINTF
OBJS+=$(top_builddir)/src/backend/port/snprintf.o

$(top_builddir)/src/backend/port/snprintf.o:
$(MAKE) -C $(top_builddir)/src/backend/port snprintf.o
endif

Here we see SNPRINTF done in two different ways. I think the library
file is the way to go anyway. We compile it once, and use it whenever
we need it. Clean.

> The rule of thumb is that rules involving the C compiler should only use
> files in the current directory. Otherwise you don't know where those
> files are going to end up.

Sure.

> > > A secondary objection is that I've been meaning to replace configure
> > > checks of the form
> > >
> > > AC_CHECK_FUNCS(inet_aton, [], INET_ATON='inet_aton.o')
> > > AC_SUBST(INET_ATON)
> > >
> > > with one integrated macro, which doesn't work if we have to encode the
> > > path into configure.
> >
> > The path is only the thing we assign to the variable. I can't see how
> > that effects the configure script.
>
> What I want this to read in the end is
>
> PGAC_SOME_MACRO([func1 func2 func3])
>
> > Actually, once we move stuff into the same directory, it will not
> > matter.
>
> True, that will help a lot.

Heck, soon configure is only going to control what gets put in the
libport.a file and that's it.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian writes:

> > Can you point to one example of such an inconsistency? I can't find one.
>
> Sure, interfaces/libpq had:
>
> OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \
> pqexpbuffer.o dllist.o md5.o pqsignal.o fe-secure.o \
> $(INET_ATON) $(SNPRINTF) $(STRERROR)
>
> while psql/Makefile had what I think you wanted:

Note that the libpq makefile goes through trouble to link the inet_aton.c
file into the current directory, so this example doesn't count.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> Bruce Momjian writes:
>
> > > Can you point to one example of such an inconsistency? I can't find one.
> >
> > Sure, interfaces/libpq had:
> >
> > OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \
> > pqexpbuffer.o dllist.o md5.o pqsignal.o fe-secure.o \
> > $(INET_ATON) $(SNPRINTF) $(STRERROR)
> >
> > while psql/Makefile had what I think you wanted:
>
> Note that the libpq makefile goes through trouble to link the inet_aton.c
> file into the current directory, so this example doesn't count.

Well, the code is:

# this only gets done if configure finds system doesn't have inet_aton()
inet_aton.c: $(backend_src)/port/inet_aton.c
rm -f $@ && $(LN_S) $< .

How is this any better than just mentioning the *.o file and letting the
default rules compile it. I don't understand how linking to the current
directory gets us anything. Now, if you did a 'make -C dir target' that
would be different.

In fact, with the lib idea dead, if there are special rules for for
certain port/*.o files, we should put those rules in Makefile.global and
let all the code use it. That way, we defined it in one place, but can
use the object file anywhere.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> How is this any better than just mentioning the *.o file and letting the
> default rules compile it. I don't understand how linking to the current
> directory gets us anything. Now, if you did a 'make -C dir target' that
> would be different.

The whole point of the pushups for libpq is that we DON'T want the
default rules. We need to compile it PIC so that it can go into a
shared library. This will not be the same object file built in the
ports directory.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > How is this any better than just mentioning the *.o file and letting the
> > default rules compile it. I don't understand how linking to the current
> > directory gets us anything. Now, if you did a 'make -C dir target' that
> > would be different.
>
> The whole point of the pushups for libpq is that we DON'T want the
> default rules. We need to compile it PIC so that it can go into a
> shared library. This will not be the same object file built in the
> ports directory.

Oh, so specifying the full path for the *.o allows it to use an existing
*.o, while putting it in the directory forces a recompile with the
current Makefile. Got it. Libpq has all of them done.

I see the comment in libpq/Makefile now:

# We use several backend modules verbatim, but since we need to
# compile with appropriate options to build a shared lib, we can't
# necessarily use the same object files as the backend uses. Instead,
# symlink the source files in here and build our own object file.

Thanks.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > How is this any better than just mentioning the *.o file and letting the
> > default rules compile it. I don't understand how linking to the current
> > directory gets us anything. Now, if you did a 'make -C dir target' that
> > would be different.
>
> The whole point of the pushups for libpq is that we DON'T want the
> default rules. We need to compile it PIC so that it can go into a
> shared library. This will not be the same object file built in the
> ports directory.

OK, this cleanup makes the src/backend/port file location dependent only
on configure.in values. I will now move them from src/backend/port to
src/port.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > How is this any better than just mentioning the *.o file and letting the
> > default rules compile it. I don't understand how linking to the current
> > directory gets us anything. Now, if you did a 'make -C dir target' that
> > would be different.
>
> The whole point of the pushups for libpq is that we DON'T want the
> default rules. We need to compile it PIC so that it can go into a
> shared library. This will not be the same object file built in the
> ports directory.

OK, I have moved the files to src/port. Would people like this rule
added to Makefile.global.in so that any usage of src/port/*.c files will
compile in the local directory?

ifdef SNPRINTF
$(basename $(notdir $(SNPRINTF))).c: $(basename $(SNPRINTF)).c
rm -f $@ && $(LN_S) $< .
endif

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> OK, I have moved the files to src/port. Would people like this rule
> added to Makefile.global.in so that any usage of src/port/*.c files will
> compile in the local directory?

I'd guess *not*. libpq is a special case.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > OK, I have moved the files to src/port. Would people like this rule
> > added to Makefile.global.in so that any usage of src/port/*.c files will
> > compile in the local directory?
>
> I'd guess *not*. libpq is a special case.

But the case is specifically for libpq, so that it appears as a local
file for compile by the local makefile rules. Adding this would make
all uses of those files behave the same way.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > OK, I have moved the files to src/port. Would people like this rule
> > added to Makefile.global.in so that any usage of src/port/*.c files will
> > compile in the local directory?
>
> I'd guess *not*. libpq is a special case.

Oh, so you are saying let most uses of src/port use the *.o files that
are in the directory, and it isn't needed to have other directories use
the link trick. Just let me know what people want.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian writes:

> Oh, so you are saying let most uses of src/port use the *.o files that
> are in the directory, and it isn't needed to have other directories use
> the link trick. Just let me know what people want.

What I want is this:

In configure.in, call

AC_REPLACE_FUNCS([inet_aton ...]) # the whole list

If you need more sophisticated checks on top of "function exists", you
keep the existing tests, but instead of, say,

SNPRINTF=snprintf.c
AC_SUBST(SNPRINTF)

you'd call

AC_LIBOBJ(snprintf)

In Makefile.global.in:

LIBOBJS = @LIBOBJS@

In utils/port/Makefile:

libpgport.a: $(LIBOBJS)
ar crs $@ $^

In Makefile.global.in:

Add -L$(top_builddir)/src/port to LDFLAGS (near the start), -lpgport to
LIBS (near the end).

Then you need to make sure that the src/port directory is build before
being referred to.

In the libpq makefile, you can write the rules like:

ifneq(,$(filter snprintf.o, $(LIBOBJS)))
# do what it's doing now in case of 'ifdef SNPRINF'
endif

Caveat implementor.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Wow. I think I will hold for a while.

---------------------------------------------------------------------------

Peter Eisentraut wrote:
> Bruce Momjian writes:
>
> > Oh, so you are saying let most uses of src/port use the *.o files that
> > are in the directory, and it isn't needed to have other directories use
> > the link trick. Just let me know what people want.
>
> What I want is this:
>
> In configure.in, call
>
> AC_REPLACE_FUNCS([inet_aton ...]) # the whole list
>
> If you need more sophisticated checks on top of "function exists", you
> keep the existing tests, but instead of, say,
>
> SNPRINTF=snprintf.c
> AC_SUBST(SNPRINTF)
>
> you'd call
>
> AC_LIBOBJ(snprintf)
>
> In Makefile.global.in:
>
> LIBOBJS = @LIBOBJS@
>
> In utils/port/Makefile:
>
> libpgport.a: $(LIBOBJS)
> ar crs $@ $^
>
> In Makefile.global.in:
>
> Add -L$(top_builddir)/src/port to LDFLAGS (near the start), -lpgport to
> LIBS (near the end).
>
> Then you need to make sure that the src/port directory is build before
> being referred to.
>
> In the libpq makefile, you can write the rules like:
>
> ifneq(,$(filter snprintf.o, $(LIBOBJS)))
> # do what it's doing now in case of 'ifdef SNPRINF'
> endif
>
>
> Caveat implementor.
>
> --
> Peter Eisentraut peter_e(at)gmx(dot)net
>
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026