Re: patch for AIX system
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 17, 2010, 17:51 UTC
- Message-ID
- <7vr5ejg7oi.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4CE38472.8070206@Nemec-online.de>
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.
Show 20 quoted lines
> --- > 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...
Show 13 quoted lines
> 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)