On Mon, Jul 6, 2009 at 7:12 PM, Matt Helsley<[email protected]> wrote:
> (Reviewing inline patches is much easier..)

Ok. I didn't know what the procedure was for submitting patches to the
group, because it doesn't mention it in the DCO, but I'll do that next
round.

> Most of this looks good to me.
>
> Introducing include/mk seems like a very good idea -- incredible numbers of
> Makefiles could be factored into common includes there. It would
> probably make maintenance of the LTP Makefiles alot easier in the long
> run.

Bingo sir ;)! That's why I was asking whether or not I could do away
with subdir makes -- it would make life for me as a developer a lot
easier, because we have to backtrack with subdir ?= [...] in the
Makefiles and could just use -I/path/to/srcdir . If we were using a
wrapper script, like bash, perl, or python, etc then we could _easily_
get away with having to deal with the srcdir stuff because we could
walk up the tree, but I'd rather not go that route :\...

> On Mon, Jul 06, 2009 at 06:18:10PM -0700, Garrett Cooper wrote:
>> 1. Many Makefiles have hardcoded /opt/ltp paths in them. Let's make
>> things more modular by making sure the files either get parsed by
>> autoconf with configure, or instead of having the hardcoded path,
>> include include/mk/config.mk (which is in and of itself a configure'd
>> file), thus ensuring feature parity in all of our Makefiles.
>> 2. These files also contain various cleanup items with other vars and
>> rules conforming to the Draft 1 spec of master_rules.mk.
>>
>> This requires PATCH 2/4 and PATCH 3/4.
>>
>> Signed-off-by: Garrett Cooper <[email protected]>

[...]

> Shouldn't the old files be left as defaults so we don't need to run
> autotools to get a working make/install?
>
> <snip>

Sure. I could do that with a large chunk of the generated files, but I
was just trying to avoid it as much as possible.

>> Index: include/mk/config.mk.in
>> ===================================================================
>> RCS file: include/mk/config.mk.in
>> diff -N include/mk/config.mk.in
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ include/mk/config.mk.in   7 Jul 2009 01:15:48 -0000
>> @@ -0,0 +1,61 @@
>> +#
>> +#    config.mk.in.
>> +#
>> +#    Copyright (C) 2009, Cisco Systems Inc.
>> +#
>> +#    This program is free software; you can redistribute it and/or modify
>> +#    it under the terms of the GNU General Public License as published by
>> +#    the Free Software Foundation; either version 2 of the License, or
>> +#    (at your option) any later version.
>> +#
>> +#    This program is distributed in the hope that it will be useful,
>> +#    but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +#    GNU General Public License for more details.
>> +#
>> +#    You should have received a copy of the GNU General Public License along
>> +#    with this program; if not, write to the Free Software Foundation, Inc.,
>> +#    51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> +#
>> +# Garrett Cooper, July 2009
>> +#
>> +# Copyright (c) 2009 by Cisco Systems, Inc.
>> +# All rights reserved
>> +#
>> +
>> +AR           = @AR@
>> +CC           = @CC@
>> +RANLIB               = @RANLIB@
>> +
>> +# Avoid overwriting definitions in leaf callers.
>> +CPPFLAGS     ?= @CPPFLAGS@
>> +CFLAGS               ?= @CFLAGS@
>> +LDLIBS               ?= @LIBS@
>> +LDFLAGS              ?= @LDFLAGS@
>> +
>> +AIO_LIBS     = @AIO_LIBS@
>> +CRYPTO_LIB   = @CRYPTO_LIB@
>> +SELINUX_LIBS = @SELINUX_LIBS@
>> +
>> +RPMBUILD     ?= rpmbuild
>> +
>> +prefix               ?= @prefix@
>
> I think you should set PREFIX too. Most Makefiles that honored prefix at
> all expected that variable name. Perhaps it would be best to start with
> that and then, if it's best, gradually change all the Makefiles?

Sure -- will add that then too :).

>> +
>> +#
>> +# Default to /opt/ltp if the directory specified was a zero-length string, 
>> to
>> +# avoid spurious Make system errors.
>> +#
>> +# XXX (garrcoop): I'm almost positive there's a better way to do this in
>> +# autoconf. Haven't figured it out yet...
>> +#
>> +ifeq ($(strip $(prefix)),)
>> +prefix               = /opt/ltp
>> +endif
>
> I thought the point was not having to run autoconf to get the default
> build and install via Makefiles.

Yeah, I thought about it and wondered to myself -- how many folks are
there out there that actually handcraft config.h and config.mk files.
Probably not that many folks to be honest...

>> +
>> +datarootdir  = @datarootdir@
>> +includedir   = @includedir@
>> +mandir               = @mandir@
>> +
>> +#export AR CC CFLAGS CPPFLAGS LDLIBS LDFLAGS RANLIB
>> +export AR CC CPPFLAGS LDLIBS LDFLAGS RANLIB
>
> Perhaps now is a good time to delete the commented-out export line?

Yeah, silly me -- forgot about that =].

Thanks!
-Garrett

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have 
the opportunity to enter the BlackBerry Developer Challenge. See full prize 
details at: http://p.sf.net/sfu/blackberry
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Reply via email to