From: Ævar Arnfjörð Bjarmason Date: Sun, 14 Nov 2010 17:51:26 GMT Subject: Re: [PATCH 1/4] Makefile: move "Platform specific tweaks" above LIB_{H,OBJS} Message-ID: In-Reply-To: <20101114172331.GA26459@burratino> On Sun, Nov 14, 2010 at 18:23, Jonathan Nieder wrote: > Ævar Arnfjörð Bjarmason wrote: > >> Change the Makefile so that the "Platform specific tweaks" section >> comes before the assignments to LIB_H and LIB_OBJS. > > Currently the Makefile is structured like this: > >        A. default target >        B. basics >           1. basic configuration section >              a. -include GIT-VERSION-FILE and recipe to generate it >              b. uname_S := $(shell uname -s) and similar variables >              c. user-facing compilation variables: CFLAGS, LDFLAGS, STRIP >              d. user-facing paths: prefix, bindir_relative, etc >              e. program names: CC, AR, etc >           2. basic cflags and ldflags (almost configurable) >           3. main list of program targets: >              SCRIPTS, PROGRAMS, TEST_PROGRAMS, BUILT_INS, >              OTHER_PROGRAMS, BINDIR_PROGRAMS >           4. defaults for SHELL_PATH, PERL_PATH, PYTHON_PATH >           5. main list of library targets: >              LIB_FILE, XDIFF_LIB, LIB_H, LIB_OBJS, BUILTIN_OBJS >           6. GITLIBS, EXTLIBS for the linker command line >           7. platform-specific tweaks >           8. -include config.mak, config.mak.autogen >        C. preparations >           1. handling of the various NO_THIS_OR_THAT options. >              This affects BASIC_CFLAGS, COMPAT_CFLAGS, >              COMPAT_OBJS, PROGRAMS, EXTLIBS, LIB_OBJ, LIB_H, etc >           2. machinery for non-noisy build >           3. shell-quoted and C-quoted variables >           4. ALL_CFLAGS, ALL_LDFLAGS >        D. main build rules >           1. all:: targets for the main build, subdirs >           2. shell sanity check >           3. building the git binary and built-ins >           4. scripts and gitweb >           5. autoconf >           6. building objects: >              a. %.o: %.c rule, header deps, dependency checking >              b. target-specific -D flags >           7. building non-builtins, remote-curl >           8. libs >           9. subdirs >        E. GIT-CFLAGS, GIT-BUILD-OPTIONS, GIT-GUI-VARS >        F. bin-wrappers >        G. tests >        H. installation rules >        I. maintainer's dist rules, check-doc, coverage, etc That listing should be in a comment at the start of the Makefile. Please submit a patch for that! > This patch proposes moving A5 (main list of library targets) after > A8 (end of configuration). Right. Thanks for the helpful outline. >> In the ab/i18n series I only want to build gettext.o (by adding it to >> LIB_OBJS) if NO_GETTEXT is unset. It's not possible to do that without >> an ugly hack if we haven't applied our platform specific tweaks before >> LIB_{H,OBJS} gets assigned to. >> >> See <201008140002.40587.j6t@kdbg.org> (subject: "[PATCH] Do not build >> i18n on Windows.") for Johannes's original report, and my follow-up in >> where I >> suggested that the problem be solved in the manner of this patch. > > This doesn't motivate the patch all all to me.  Is changing the list > of LIB_OBJS in section C1 really an ugly hack?  It is where > configuration-specific things go and how BLK_SHA1, PROGRAM_OBJS, etc > work already. Yeah, because if it hadn't come before A8 it would have been *not* assigned to in the first place like we do everywhere else. Moving A8 before A5 enables us to do that. > That said, I can see another reason to move A3 and A5 lower down in > the makefile.  Namely, they don't seem to have anything obvious to > do with configuration.  Including the basic list of objects that > high up may make the makefile easier to read straight through, but > I don't think anyone is reading it straight through. > > So I wouldn't have anything against moving both A3 and A5 to right > before C1, I just think it needs different motivation. I was thinking about moving A3 & A5 further down. But since I just needed A5 now I only ended up doing that. Moving them both would clean things up though. I think we should do A8 as early as possible.