Re: patch for AIX system

[Norbert Nemec <Norbert@Nemec-online.de>]

Sorry about that and thanks for the patient reply. I'll crank up my 
standard of quality and address the issues with another submission.

Greetings,
Norbert


On 11/17/2010 06:51 PM, Junio C Hamano wrote:
> Norbert Nemec<Norbert@Nemec-online.de>  writes:
>
>> Find attached a small patch to make git install cleanly on an AIX system
> A few comments, some from Documentation/SubmittingPatches.
>
>>  From def4428ad827d1e6550634a2fe1f035c1b148426 Mon Sep 17 00:00:00 2001
>> From: Norbert Nemec<Norbert@Nemec-online.de>
>> Date: Wed, 17 Nov 2010 08:25:44 +0100
>> Subject: [PATCH] Fix for installation on AIX
> Please don't do this; instead, make sure that From/Date/Subject of your
> e-mail usable as the metainfo for the resulting commit (in this case only
> the Subject needs a change) and drop these lines, i.e. make the patch
> inline, not an attachment.
>
> Also make the subject more specific, to make it clear _what_ you fixed;
> phrase it like "Makefile: On AIX, bsdinstall does not understand -d" or
> something, perhaps.
>
>> The BSD style 'install' command is call 'installbsd' and does not support the -d option.
>> Therefore '$(INSTALL) -d' is replaced by a new variable '$(INSTALLDIR)' in all Makefiles
>> which can be changed independently of $(INSTALL).
> These lines are tad too long; please wrap at around 66-72 cols.
>
> Missing sign-off.
>
>> ---
>>   Documentation/Makefile |   10 +++++-----
>>   Makefile               |    7 +++++--
>>   git-gui/Makefile       |    4 ++--
>>   gitk-git/Makefile      |    2 +-
>>   gitweb/Makefile        |    4 ++--
>>   templates/Makefile     |    2 +-
>>   6 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index e117bc4..c2db8db 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> ...
>>   install-pdf: pdf
>> -	$(INSTALL) -d -m 755 $(DESTDIR)$(pdfdir)
>> +	$(INSTALLDIR) -m 755 $(DESTDIR)$(pdfdir)
>>   	$(INSTALL) -m 644 user-manual.pdf $(DESTDIR)$(pdfdir)
>>
>>   install-html: html
> In the main Makefile, we have this line:
>
> 	export DIFF TAR INSTALL DESTDIR SHELL_PATH
>
> so that they can be used in "$(MAKE) -C Documentaiton install-frotz"
> invocations in subdirectories, but I do not see an addition of export in
> your patch, and you do not define INSTALLDIR in Documentation/Makefile
> either.  I wonder how this could possibly work...
>
>> diff --git a/Makefile b/Makefile
>> index 1f1ce04..2664d7c 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -953,6 +954,8 @@ ifeq ($(uname_S),AIX)
>>   	NO_MKSTEMPS = YesPlease
>>   	NO_STRLCPY = YesPlease
>>   	NO_NSEC = YesPlease
>> +	INSTALL = installbsd
>> +	INSTALLDIR = mkdir -p
>>   	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>>   	INTERNAL_QSORT = UnfortunatelyYes
>>   	NEEDS_LIBICONV=YesPlease
> I'd defer this section to AIX experts; I've always assumed that people
> on non-gnu platforms used ginstall, but perhaps AIX doesn't have one?
>
> Other changes looked mostly OK, but I am still wondering how your
> INSTALLDIR is passed down to submakes.
>
>   ... spends quality 30-minutes digging ...
>
> No, your patch does not work.  Have you even tested it?
>
> The attached patch on top of yours makes things working, it seems.
>
> Points to note:
>
>   - People may have already used "make INSTALL=ginstall" and been expecting
>     it to work.  Defining "INSTALLDIR = install -d" as the default will
>     break them (see how I defined it in the attached);
>
>   - "cd gitweb&&  make" is supposed to work, so you would need fallback
>     definitions in Makefiles in subdirectories; and
>
>   - When coming from the toplevel make, submakes do want the INSTALLDIR
>     definition passed from it (see how "export" is used).
>
> Thanks.
>
>   Documentation/Makefile |    3 ++-
>   Makefile               |    4 ++--
>   git-gui/Makefile       |    3 +++
>   gitk-git/Makefile      |    1 +
>   gitweb/Makefile        |    1 +
>   templates/Makefile     |    1 +
>   6 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index c2db8db..5c8aacc 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -44,7 +44,8 @@ ASCIIDOC=asciidoc
>   ASCIIDOC_EXTRA =
>   MANPAGE_XSL = manpage-normal.xsl
>   XMLTO_EXTRA =
> -INSTALL?=install
> +INSTALL ?= install
> +INSTALLDIR ?= $(INSTALL) -d
>   RM ?= rm -f
>   DOC_REF = origin/man
>   HTML_REF = origin/html
> diff --git a/Makefile b/Makefile
> index 2664d7c..b91de40 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -308,7 +308,7 @@ DIFF = diff
>   TAR = tar
>   FIND = find
>   INSTALL = install
> -INSTALLDIR = install -d
> +INSTALLDIR = $(INSTALL) -d
>   RPMBUILD = rpmbuild
>   TCL_PATH = tclsh
>   TCLTK_PATH = wish
> @@ -1573,7 +1573,7 @@ endif
>   ALL_CFLAGS += $(BASIC_CFLAGS)
>   ALL_LDFLAGS += $(BASIC_LDFLAGS)
>
> -export DIFF TAR INSTALL DESTDIR SHELL_PATH
> +export DIFF TAR INSTALL INSTALLDIR DESTDIR SHELL_PATH
>
>
>   ### Build rules
> diff --git a/git-gui/Makefile b/git-gui/Makefile
> index b96b3df..be8bd2d 100644
> --- a/git-gui/Makefile
> +++ b/git-gui/Makefile
> @@ -44,6 +44,9 @@ endif
>   ifndef INSTALL
>   	INSTALL = install
>   endif
> +ifndef INSTALLDIR
> +	INSTALLDIR = $(INSTALL) -d
> +endif
>
>   RM_RF     ?= rm -rf
>   RMDIR     ?= rmdir
> diff --git a/gitk-git/Makefile b/gitk-git/Makefile
> index b838d87..473c500 100644
> --- a/gitk-git/Makefile
> +++ b/gitk-git/Makefile
> @@ -11,6 +11,7 @@ msgsdir_SQ  = $(subst ','\'',$(msgsdir))
>   TCL_PATH ?= tclsh
>   TCLTK_PATH ?= wish
>   INSTALL ?= install
> +INSTALLDIR ?= $(INSTALL) -d
>   RM ?= rm -f
>
>   DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index 3f1ac82..ee6dcc1 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -16,6 +16,7 @@ gitwebdir ?= /var/www/cgi-bin
>
>   RM ?= rm -f
>   INSTALL ?= install
> +INSTALLDIR ?= $(INSTALL) -d
>
>   # default configuration for gitweb
>   GITWEB_CONFIG = gitweb_config.perl
> diff --git a/templates/Makefile b/templates/Makefile
> index 3808b04..953037c 100644
> --- a/templates/Makefile
> +++ b/templates/Makefile
> @@ -5,6 +5,7 @@ ifndef V
>   endif
>
>   INSTALL ?= install
> +INSTALLDIR ?= $(INSTALL) -d
>   TAR ?= tar
>   RM ?= rm -f
>   prefix ?= $(HOME)


-- 
_______________________________________________Norbert Nemec
   Lilienstr. 5a ... 12203 Berlin-Lichterfelde ... Germany
       Tel: +49-30-5483 3143 Mobile: +49-176-5502 5643
           eMail:<Norbert@Nemec-online.de>