threads / patch / 32891

patch, 3 partsDocumentation/Makefile: fix inherited {html,info,man}dir

Subject: [PATCH 3/3] Documentation/Makefile: fix inherited {html,info,man}dir

## tl;dr

20 messages between Feb 12, 2013 and Feb 25, 2013. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

John Keeping· Feb 12, 2013, 20:17 UTC · lore

[PATCH 0/3] Fix installation paths with "make install-doc"

When using the top-level install-doc target the html, info and man target directories are inherited from the top-level Makefile by the documentation Makefile as relative paths, which is not expected and results in the files being installed in an unexpected location.

The first two patches are simple style fixes. The third one fixes the issue described above.

John Keeping (3):
  Documentation/Makefile: fix spaces around assignments
  Documentation/Makefile: move infodir to be with other '*dir's
  Documentation/Makefile: fix inherited {html,info,man}dir
 Documentation/Makefile | 88 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 56 insertions(+), 32 deletions(-)
-- 
1.8.1.2
John Keeping· Feb 12, 2013, 20:17 UTC · re: John Keeping · lore

[PATCH 1/3] Documentation/Makefile: fix spaces around assignments

A simple style fix; no functional change.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/Makefile | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)
Show changes to Documentation/Makefile +21 −21
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 62dbd9a..af3d8a4 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -31,11 +31,11 @@ MAN7_TXT += gittutorial.txt
 MAN7_TXT += gitworkflows.txt
 
 MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
-MAN_XML=$(patsubst %.txt,%.xml,$(MAN_TXT))
-MAN_HTML=$(patsubst %.txt,%.html,$(MAN_TXT))
+MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))
+MAN_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
 
 OBSOLETE_HTML = git-remote-helpers.html
-DOC_HTML=$(MAN_HTML) $(OBSOLETE_HTML)
+DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
 
 ARTICLES = howto-index
 ARTICLES += everyday
@@ -74,35 +74,35 @@ SP_ARTICLES += technical/api-index
 
 DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
 
-DOC_MAN1=$(patsubst %.txt,%.1,$(MAN1_TXT))
-DOC_MAN5=$(patsubst %.txt,%.5,$(MAN5_TXT))
-DOC_MAN7=$(patsubst %.txt,%.7,$(MAN7_TXT))
+DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
+DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
+DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
 
-prefix?=$(HOME)
-bindir?=$(prefix)/bin
-htmldir?=$(prefix)/share/doc/git-doc
-pdfdir?=$(prefix)/share/doc/git-doc
-mandir?=$(prefix)/share/man
-man1dir=$(mandir)/man1
-man5dir=$(mandir)/man5
-man7dir=$(mandir)/man7
-# DESTDIR=
+prefix ?= $(HOME)
+bindir ?= $(prefix)/bin
+htmldir ?= $(prefix)/share/doc/git-doc
+pdfdir ?= $(prefix)/share/doc/git-doc
+mandir ?= $(prefix)/share/man
+man1dir = $(mandir)/man1
+man5dir = $(mandir)/man5
+man7dir = $(mandir)/man7
+# DESTDIR =
 
 ASCIIDOC = asciidoc
 ASCIIDOC_EXTRA =
 MANPAGE_XSL = manpage-normal.xsl
 XMLTO = xmlto
 XMLTO_EXTRA =
-INSTALL?=install
+INSTALL ?= install
 RM ?= rm -f
 MAN_REPO = ../../git-manpages
 HTML_REPO = ../../git-htmldocs
 
-infodir?=$(prefix)/share/info
-MAKEINFO=makeinfo
-INSTALL_INFO=install-info
-DOCBOOK2X_TEXI=docbook2x-texi
-DBLATEX=dblatex
+infodir ?= $(prefix)/share/info
+MAKEINFO = makeinfo
+INSTALL_INFO = install-info
+DOCBOOK2X_TEXI = docbook2x-texi
+DBLATEX = dblatex
 ifndef PERL_PATH
 	PERL_PATH = /usr/bin/perl
 endif
-- 
1.8.1.2
Jonathan Nieder· Feb 12, 2013, 21:00 UTC · re: John Keeping · lore

Re: [PATCH 1/3] Documentation/Makefile: fix spaces around assignments

Hi,
John Keeping wrote:
> [Subject: [PATCH 1/3] Documentation/Makefile: fix spaces around assignments]
It's not so much "fix spaces" as "use consistent spacing", no?
Aside from that nit, looks like a sensible no-op to me, so
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
John Keeping· Feb 12, 2013, 20:17 UTC · re: John Keeping · lore

[PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index af3d8a4..0cfdc36 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin
 htmldir ?= $(prefix)/share/doc/git-doc
+infodir ?= $(prefix)/share/info
 pdfdir ?= $(prefix)/share/doc/git-doc
 mandir ?= $(prefix)/share/man
 man1dir = $(mandir)/man1
@@ -98,7 +99,6 @@ RM ?= rm -f
 MAN_REPO = ../../git-manpages
 HTML_REPO = ../../git-htmldocs
 
-infodir ?= $(prefix)/share/info
 MAKEINFO = makeinfo
 INSTALL_INFO = install-info
 DOCBOOK2X_TEXI = docbook2x-texi
-- 
1.8.1.2
Jonathan Nieder· Feb 12, 2013, 21:01 UTC · re: John Keeping · lore

Re: [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's

John Keeping wrote:
> Signed-off-by: John Keeping <john@keeping.me.uk>
[...]
Show 16 quoted lines
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
>  prefix ?= $(HOME)
>  bindir ?= $(prefix)/bin
>  htmldir ?= $(prefix)/share/doc/git-doc
> +infodir ?= $(prefix)/share/info
>  pdfdir ?= $(prefix)/share/doc/git-doc
>  mandir ?= $(prefix)/share/man
>  man1dir = $(mandir)/man1
> @@ -98,7 +99,6 @@ RM ?= rm -f
>  MAN_REPO = ../../git-manpages
>  HTML_REPO = ../../git-htmldocs
>  
> -infodir ?= $(prefix)/share/info
>  MAKEINFO = makeinfo

Is this another stylefix or is there a functional reason for this change?

John Keeping· Feb 12, 2013, 21:18 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's

On Tue, Feb 12, 2013 at 01:01:38PM -0800, Jonathan Nieder wrote:
Show 23 quoted lines
> John Keeping wrote:
> 
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> [...]
> > --- a/Documentation/Makefile
> > +++ b/Documentation/Makefile
> > @@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
> >  prefix ?= $(HOME)
> >  bindir ?= $(prefix)/bin
> >  htmldir ?= $(prefix)/share/doc/git-doc
> > +infodir ?= $(prefix)/share/info
> >  pdfdir ?= $(prefix)/share/doc/git-doc
> >  mandir ?= $(prefix)/share/man
> >  man1dir = $(mandir)/man1
> > @@ -98,7 +99,6 @@ RM ?= rm -f
> >  MAN_REPO = ../../git-manpages
> >  HTML_REPO = ../../git-htmldocs
> >  
> > -infodir ?= $(prefix)/share/info
> >  MAKEINFO = makeinfo
> 
> Is this another stylefix or is there a functional reason for this
> change?

Another stylefix - this arrangement seems more logical to me and makes the comment in the next patch simpler.

John Keeping· Feb 12, 2013, 20:17 UTC · re: John Keeping · lore
Commit e14421b (Allow INSTALL, bindir, mandir to be set in main Makefile
- 2006-06-29) changed Documentation/Makefile to inherit the value of
mandir from the top-level Makefile when invoked as "make install-doc" at
the top-level.  This was inherited by infodir and htmldir when they were
added.

This was broken by commit 026fa0d (Move computation of absolute paths from Makefile to runtime (in preparation for RUNTIME_PREFIX) - 2009-01-18) which changed these variables to have relative paths in the top-level Makefile, causing the documentation to be installed into the path without $(prefix) prepended.

Fix this by changing the defaults to be paths relative to $(prefix) and introducing new variables {html,info,man}_instdir which contain the full installation paths.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
I'm not sure if this is the best approach - the alternative would be to
change the top-level Makefile to use {html,info,man}dir_relative and
derive the {html,info,man}dir variables from that.

The top-level Makefile is inconsistent in the approach it takes - bindir is derived from bindir_relative but gitexecdir and template_dir have gitexec_instdir and template_instdir derived from them.

 Documentation/Makefile | 56 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 16 deletions(-)
Show changes to Documentation/Makefile +40 −16
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 0cfdc36..34cd9f2 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -78,15 +78,21 @@ DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
 DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
 DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
 
+# The following variables can be relative paths due to the way they can be
+# inherited from the top-level Makefile:
+#   htmldir
+#   infodir
+#   mandir
+# Note that pdfdir is an exception to this since it is not used by git-help.
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin
-htmldir ?= $(prefix)/share/doc/git-doc
-infodir ?= $(prefix)/share/info
 pdfdir ?= $(prefix)/share/doc/git-doc
-mandir ?= $(prefix)/share/man
-man1dir = $(mandir)/man1
-man5dir = $(mandir)/man5
-man7dir = $(mandir)/man7
+htmldir ?= share/doc/git-doc
+infodir ?= share/info
+mandir ?= share/man
+man1dir = $(man_instdir)/man1
+man5dir = $(man_instdir)/man5
+man7dir = $(man_instdir)/man7
 # DESTDIR =
 
 ASCIIDOC = asciidoc
@@ -110,6 +116,24 @@ endif
 -include ../config.mak.autogen
 -include ../config.mak
 
+ifneq ($(filter /%,$(firstword $(htmldir))),)
+html_instdir = $(htmldir)
+else
+html_instdir = $(prefix)/$(htmldir)
+endif
+
+ifneq ($(filter /%,$(firstword $(infodir))),)
+info_instdir = $(infodir)
+else
+info_instdir = $(prefix)/$(infodir)
+endif
+
+ifneq ($(filter /%,$(firstword $(mandir))),)
+man_instdir = $(mandir)
+else
+man_instdir = $(prefix)/$(mandir)
+endif
+
 #
 # For docbook-xsl ...
 #	-1.68.1,	no extra settings are needed?
@@ -144,7 +168,7 @@ endif
 # Distros may want to use MAN_BASE_URL=file:///path/to/git/docs/
 # or similar.
 ifndef MAN_BASE_URL
-MAN_BASE_URL = file://$(htmldir)/
+MAN_BASE_URL = file://$(html_instdir)/
 endif
 XMLTO_EXTRA += -m manpage-base-url.xsl
 
@@ -220,13 +244,13 @@ install-man: man
 	$(INSTALL) -m 644 $(DOC_MAN7) $(DESTDIR)$(man7dir)
 
 install-info: info
-	$(INSTALL) -d -m 755 $(DESTDIR)$(infodir)
-	$(INSTALL) -m 644 git.info gitman.info $(DESTDIR)$(infodir)
-	if test -r $(DESTDIR)$(infodir)/dir; then \
-	  $(INSTALL_INFO) --info-dir=$(DESTDIR)$(infodir) git.info ;\
-	  $(INSTALL_INFO) --info-dir=$(DESTDIR)$(infodir) gitman.info ;\
+	$(INSTALL) -d -m 755 $(DESTDIR)$(info_instdir)
+	$(INSTALL) -m 644 git.info gitman.info $(DESTDIR)$(info_instdir)
+	if test -r $(DESTDIR)$(info_instdir)/dir; then \
+	  $(INSTALL_INFO) --info-dir=$(DESTDIR)$(info_instdir) git.info ;\
+	  $(INSTALL_INFO) --info-dir=$(DESTDIR)$(info_instdir) gitman.info ;\
 	else \
-	  echo "No directory found in $(DESTDIR)$(infodir)" >&2 ; \
+	  echo "No directory found in $(DESTDIR)$(info_instdir)" >&2 ; \
 	fi
 
 install-pdf: pdf
@@ -234,7 +258,7 @@ install-pdf: pdf
 	$(INSTALL) -m 644 user-manual.pdf $(DESTDIR)$(pdfdir)
 
 install-html: html
-	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
+	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(html_instdir)
 
 ../GIT-VERSION-FILE: FORCE
 	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
@@ -402,14 +426,14 @@ require-manrepo::
 	then echo "git-manpages repository must exist at $(MAN_REPO)"; exit 1; fi
 
 quick-install-man: require-manrepo
-	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO) $(DESTDIR)$(mandir)
+	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO) $(DESTDIR)$(man_instdir)
 
 require-htmlrepo::
 	@if test ! -d $(HTML_REPO); \
 	then echo "git-htmldocs repository must exist at $(HTML_REPO)"; exit 1; fi
 
 quick-install-html: require-htmlrepo
-	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REPO) $(DESTDIR)$(htmldir)
+	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REPO) $(DESTDIR)$(html_instdir)
 
 print-man1:
 	@for i in $(MAN1_TXT); do echo $$i; done
-- 
1.8.1.2
John Keeping· Feb 12, 2013, 22:36 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

On Tue, Feb 12, 2013 at 02:25:08PM -0800, Jonathan Nieder wrote:
Show 5 quoted lines
> John Keeping wrote:
> 
> >   Documentation/Makefile: fix inherited {html,info,man}dir
> 
> This doesn't seem to have hit the list.
Hmm... it made it to gmane:
http://article.gmane.org/gmane.comp.version-control.git/216188
Jonathan Nieder· Feb 12, 2013, 22:39 UTC · re: John Keeping · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

John Keeping wrote:
> On Tue, Feb 12, 2013 at 02:25:08PM -0800, Jonathan Nieder wrote:
>> John Keeping wrote:
Show 7 quoted lines
>>>   Documentation/Makefile: fix inherited {html,info,man}dir
>>
>> This doesn't seem to have hit the list.
>
> Hmm... it made it to gmane:
>
> http://article.gmane.org/gmane.comp.version-control.git/216188
So it did.  Sorry for the noise --- I'll grab it from there.

Sincerely, Jonathan

Junio C Hamano· Feb 12, 2013, 22:45 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
> John Keeping wrote:
>
>>   Documentation/Makefile: fix inherited {html,info,man}dir
>
> This doesn't seem to have hit the list.
More importantly, 
>> When using the top-level install-doc target the html, info and man
>> target directories are inherited from the top-level Makefile by the
>> documentation Makefile as relative paths, which is not expected and
>> results in the files being installed in an unexpected location.

I am not sure what problem it is trying to address. During every cycle "make doc && make install-man install-html" is run for all integration branches and it didn't cause any problems.

A wild guess.  John, are you using config.mak.autogen?

I _think_ exporting mandir/html/infodir from the top-level Makefile is wrong to begin with. We should drop the "export mandir" from there.

Giving them unusual meaning (e.g. "mandir = share/man") is already bad and that needs to be fixed by limiting this "oh, on some platforms we compile-in GIT_MAN_PATH as a relative path to an unspecified place" insanity only to where -DGIT_MAN_PATH=<path> is defined. The path used there does not help the building and installation of the documentation at all, so the variable used for the purpose of giving that <path> should not be named the same way as the variable used on Documentation/Makefile to name the real path in the first place.

Perhaps rename these to runtime_{man,html,info}dir or something and make sure {man,html,info}dir are defined as the real paths whose default values begin with $(prefix)?

Junio C Hamano· Feb 12, 2013, 22:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

Junio C Hamano <gitster@pobox.com> writes:
> I _think_ exporting mandir/html/infodir from the top-level Makefile
> is wrong to begin with.  We should drop the "export mandir" from
> there.
Ah, it is this thing, isn't it?
commit d8cf908cb6012cd4dc3d1089a849daf646150c2e
Author: Junio C Hamano <gitster@pobox.com>
Date:   Sat Feb 2 17:58:49 2013 -0800
    config.mak.in: remove unused definitions
    
    When 5566771 (autoconf: Use autoconf to write installation
    directories to config.mak.autogen, 2006-07-03) introduced support
    for autoconf generated config.mak file, it added an "export" for a
    few common makefile variables, in addition to definitions of srcdir
    and VPATH.
    
    The "export" logically does not belong there.  The make variables
    like mandir, prefix, etc, should be exported to submakes for people
    who use config.mak and people who use config.mak.autogen the same
    way; if we want to get these exported, that should be in the main
    Makefile.
    
    We do use mandir and htmldir in Documentation/Makefile, so let's
    add export for them in the main Makefile instead.
    
    We may eventually want to support VPATH, and srcdir may turn out to
    be useful for that purpose, but right now nobody uses it, so it is
    useless to define them in this file.
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

config.mak.in shouldn't have exported mandir in the first place, and the commit made it worse by moving that broken export to the main Makefile, and also added an export to htmldir as well, which was totally wrong.

Let me revert that bit first.

I still think making "mandir" to have the real path in both the top-level Makefile and Documentation/Makefile and renaming the variable that is used to form the -DGIT_MAN_PATH=<path> to optionally compile in a path relative to an unspecified location that is discovered at runtime to something else is the sane thing to do, but that is a separate issue, I think.

John Keeping· Feb 12, 2013, 23:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

On Tue, Feb 12, 2013 at 02:57:25PM -0800, Junio C Hamano wrote:
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I _think_ exporting mandir/html/infodir from the top-level Makefile
> > is wrong to begin with.  We should drop the "export mandir" from
> > there.
> 
> Ah, it is this thing, isn't it?
> 
> commit d8cf908cb6012cd4dc3d1089a849daf646150c2e
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Sat Feb 2 17:58:49 2013 -0800
> 
>     config.mak.in: remove unused definitions
>     
>     When 5566771 (autoconf: Use autoconf to write installation
>     directories to config.mak.autogen, 2006-07-03) introduced support
>     for autoconf generated config.mak file, it added an "export" for a
>     few common makefile variables, in addition to definitions of srcdir
>     and VPATH.
>     
>     The "export" logically does not belong there.  The make variables
>     like mandir, prefix, etc, should be exported to submakes for people
>     who use config.mak and people who use config.mak.autogen the same
>     way; if we want to get these exported, that should be in the main
>     Makefile.
>     
>     We do use mandir and htmldir in Documentation/Makefile, so let's
>     add export for them in the main Makefile instead.
>     
>     We may eventually want to support VPATH, and srcdir may turn out to
>     be useful for that purpose, but right now nobody uses it, so it is
>     useless to define them in this file.
>     
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looks like it - I tried this for the first time today (with pu) so I didn't realise it was a recent change, and I didn't think to blame the export line.

John
Junio C Hamano· Feb 12, 2013, 23:16 UTC · re: John Keeping · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

John Keeping <john@keeping.me.uk> writes:
> Looks like it - I tried this for the first time today (with pu) so I
> didn't realise it was a recent change, and I didn't think to blame the
> export line.

Unfortunately that bogus change is already in 'next', but luckily we caught it before it graduated to 'master' ;-)

I'll queue the regression fix patch (already sent separately); the patch should bring everything in Makefile back to the same state as in 'master', so in that sense it fixes the regression, but it does not address the real cause of the confusion, solution to which is hinted in the log message of the patch.

Thanks for catching this.
Junio C Hamano· Feb 12, 2013, 23:09 UTC · re: Junio C Hamano · lore

[PATCH] Makefile: do not export mandir/htmldir/infodir

These are defined in the main Makefile to be funny values that are optionally relative to an unspecified location that is determined at runtime. They are only suitable for hardcoding in the binary via the -DGIT_{MAN,HTML,INFO}_PATH=<value> C preprocessor options, and are not real paths, contrary to what any sane person, and more importantly, the Makefile in the documentation directory, would expect.

A longer term fix is to introduce runtime_{man,html,info}dir variables to hold these funny values, and make {man,html,info}dir variables to have real paths whose default values begin with $(prefix), but as a first step, stop exporting them from the top-level Makefile.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index 1dae2c5..26b697d 100644
--- a/Makefile
+++ b/Makefile
@@ -359,7 +359,7 @@ lib = lib
 # DESTDIR=
 pathsep = :
 
-export prefix bindir sharedir mandir htmldir sysconfdir gitwebdir localedir
+export prefix bindir sharedir sysconfdir gitwebdir localedir
 
 CC = cc
 AR = ar
-- 
1.8.1.3.675.g524d3b9
Jonathan Nieder· Feb 12, 2013, 23:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: do not export mandir/htmldir/infodir

Junio C Hamano wrote:
> These are defined in the main Makefile to be funny values that are
> optionally relative to an unspecified location that is determined at
> runtime.
[...]
> A longer term fix is to introduce runtime_{man,html,info}dir variables
> to hold these funny values, and make {man,html,info}dir variables
> to have real paths whose default values begin with $(prefix), but
> as a first step, stop exporting them from the top-level Makefile.
Makes sense.
Reported-by: John Keeping <john@keeping.me.uk>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
John Keeping· Feb 12, 2013, 23:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: do not export mandir/htmldir/infodir

On Tue, Feb 12, 2013 at 03:16:51PM -0800, Jonathan Nieder wrote:
Show 15 quoted lines
> Junio C Hamano wrote:
> 
> > These are defined in the main Makefile to be funny values that are
> > optionally relative to an unspecified location that is determined at
> > runtime.
> [...]
> > A longer term fix is to introduce runtime_{man,html,info}dir variables
> > to hold these funny values, and make {man,html,info}dir variables
> > to have real paths whose default values begin with $(prefix), but
> > as a first step, stop exporting them from the top-level Makefile.
> 
> Makes sense.
> 
> Reported-by: John Keeping <john@keeping.me.uk>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Fixes my original problem, so FWIW:
Tested-by: John Keeping <john@keeping.me.uk>
John Keeping· Feb 24, 2013, 19:55 UTC · re: Junio C Hamano · lore

[PATCH] Makefile: make mandir, htmldir and infodir absolute

This matches the use of the variables with the same names in autotools, reducing the potential for user surprise.

Using relative paths in these variables also causes issues if they are exported from the Makefile, as discussed in commit c09d62f (Makefile: do not export mandir/htmldir/infodir, 2013-02-12).

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Tue, Feb 12, 2013 at 03:09:53PM -0800, Junio C Hamano wrote:
> A longer term fix is to introduce runtime_{man,html,info}dir variables
> to hold these funny values, and make {man,html,info}dir variables
> to have real paths whose default values begin with $(prefix), but
> as a first step, stop exporting them from the top-level Makefile.
Here's an attempt at doing that.

If this is sensible, should bindir_relative be calculated in the same way?

 Makefile | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)
Show changes to Makefile +21 −15
diff --git a/Makefile b/Makefile
index 7c75e3b..087eec4 100644
--- a/Makefile
+++ b/Makefile
@@ -360,33 +360,39 @@ STRIP ?= strip
 # Among the variables below, these:
 #   gitexecdir
 #   template_dir
-#   mandir
-#   infodir
-#   htmldir
 #   sysconfdir
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
+# Additionally, the following will be treated as relative by "git" if they
+# begin with "$(prefix)/":
+#   mandir
+#   infodir
+#   htmldir
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)
 bindir_relative = bin
 bindir = $(prefix)/$(bindir_relative)
-mandir = share/man
-infodir = share/info
+mandir = $(prefix)/share/man
+infodir = $(prefix)/share/info
 gitexecdir = libexec/git-core
 mergetoolsdir = $(gitexecdir)/mergetools
 sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 localedir = $(sharedir)/locale
 template_dir = share/git-core/templates
-htmldir = share/doc/git-doc
+htmldir = $(prefix)/share/doc/git-doc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
 ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 lib = lib
 # DESTDIR =
 pathsep = :
 
+mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
+infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
+
 export prefix bindir sharedir sysconfdir gitwebdir localedir
 
 CC = cc
@@ -1548,12 +1554,12 @@ ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 bindir_SQ = $(subst ','\'',$(bindir))
 bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
-mandir_SQ = $(subst ','\'',$(mandir))
-infodir_SQ = $(subst ','\'',$(infodir))
+mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
+infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 localedir_SQ = $(subst ','\'',$(localedir))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
-htmldir_SQ = $(subst ','\'',$(htmldir))
+htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
@@ -1685,9 +1691,9 @@ strip: $(PROGRAMS) git$X
 
 git.sp git.s git.o: GIT-PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
-	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-	'-DGIT_INFO_PATH="$(infodir_SQ)"'
+	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
+	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
+	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
 git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
@@ -1697,9 +1703,9 @@ help.sp help.s help.o: common-cmds.h
 
 builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
-	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-	'-DGIT_INFO_PATH="$(infodir_SQ)"'
+	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
+	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
+	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
 version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-- 
1.8.2.rc0.248.g2dab8ff
Junio C Hamano· Feb 25, 2013, 20:35 UTC · re: John Keeping · lore

Re: [PATCH] Makefile: make mandir, htmldir and infodir absolute

John Keeping <john@keeping.me.uk> writes:
Show 20 quoted lines
> This matches the use of the variables with the same names in autotools,
> reducing the potential for user surprise.
>
> Using relative paths in these variables also causes issues if they are
> exported from the Makefile, as discussed in commit c09d62f (Makefile: do
> not export mandir/htmldir/infodir, 2013-02-12).
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> On Tue, Feb 12, 2013 at 03:09:53PM -0800, Junio C Hamano wrote:
>> A longer term fix is to introduce runtime_{man,html,info}dir variables
>> to hold these funny values, and make {man,html,info}dir variables
>> to have real paths whose default values begin with $(prefix), but
>> as a first step, stop exporting them from the top-level Makefile.
>
> Here's an attempt at doing that.
>
> If this is sensible, should bindir_relative be calculated in the same
> way?

Thanks for taking the cue from existing bindir_relative, which I overlooked. Calling these *dir_relative, not runtime_*dir as I hinted, makes a lot more sense and overall makes things consistent.

As most people would want to say bindir=/usr/local/bin and not bindir_relative=bin from the command line (and I suspect that people coming from ./configure would not even have a way to specify the latter), I think your suggestion to make bindir the primary one and derive bindir_relative from it makes sense.

What do Windows folks think?
[patch unsnipped for others' reference]
Show 95 quoted lines
>  Makefile | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 7c75e3b..087eec4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -360,33 +360,39 @@ STRIP ?= strip
>  # Among the variables below, these:
>  #   gitexecdir
>  #   template_dir
> -#   mandir
> -#   infodir
> -#   htmldir
>  #   sysconfdir
>  # can be specified as a relative path some/where/else;
>  # this is interpreted as relative to $(prefix) and "git" at
>  # runtime figures out where they are based on the path to the executable.
> +# Additionally, the following will be treated as relative by "git" if they
> +# begin with "$(prefix)/":
> +#   mandir
> +#   infodir
> +#   htmldir
>  # This can help installing the suite in a relocatable way.
>  
>  prefix = $(HOME)
>  bindir_relative = bin
>  bindir = $(prefix)/$(bindir_relative)
> -mandir = share/man
> -infodir = share/info
> +mandir = $(prefix)/share/man
> +infodir = $(prefix)/share/info
>  gitexecdir = libexec/git-core
>  mergetoolsdir = $(gitexecdir)/mergetools
>  sharedir = $(prefix)/share
>  gitwebdir = $(sharedir)/gitweb
>  localedir = $(sharedir)/locale
>  template_dir = share/git-core/templates
> -htmldir = share/doc/git-doc
> +htmldir = $(prefix)/share/doc/git-doc
>  ETC_GITCONFIG = $(sysconfdir)/gitconfig
>  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>  lib = lib
>  # DESTDIR =
>  pathsep = :
>  
> +mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
> +infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
> +htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
> +
>  export prefix bindir sharedir sysconfdir gitwebdir localedir
>  
>  CC = cc
> @@ -1548,12 +1554,12 @@ ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
>  DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
>  bindir_SQ = $(subst ','\'',$(bindir))
>  bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
> -mandir_SQ = $(subst ','\'',$(mandir))
> -infodir_SQ = $(subst ','\'',$(infodir))
> +mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
> +infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
>  localedir_SQ = $(subst ','\'',$(localedir))
>  gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
>  template_dir_SQ = $(subst ','\'',$(template_dir))
> -htmldir_SQ = $(subst ','\'',$(htmldir))
> +htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
>  prefix_SQ = $(subst ','\'',$(prefix))
>  gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
>  
> @@ -1685,9 +1691,9 @@ strip: $(PROGRAMS) git$X
>  
>  git.sp git.s git.o: GIT-PREFIX
>  git.sp git.s git.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
> -	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
> -	'-DGIT_INFO_PATH="$(infodir_SQ)"'
> +	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> +	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> +	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
> @@ -1697,9 +1703,9 @@ help.sp help.s help.o: common-cmds.h
>  
>  builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
> -	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
> -	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
> -	'-DGIT_INFO_PATH="$(infodir_SQ)"'
> +	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> +	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> +	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
>  version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
>  version.sp version.s version.o: EXTRA_CPPFLAGS = \
John Keeping· Feb 12, 2013, 23:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Fix installation paths with "make install-doc"

On Tue, Feb 12, 2013 at 02:45:34PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > John Keeping wrote:
> >
> >>   Documentation/Makefile: fix inherited {html,info,man}dir
> >
> > This doesn't seem to have hit the list.
> 
> More importantly, 
> 
> >> When using the top-level install-doc target the html, info and man
> >> target directories are inherited from the top-level Makefile by the
> >> documentation Makefile as relative paths, which is not expected and
> >> results in the files being installed in an unexpected location.
> 
> I am not sure what problem it is trying to address.  During every
> cycle "make doc && make install-man install-html" is run for all
> integration branches and it didn't cause any problems.
> 
> A wild guess.  John, are you using config.mak.autogen?
Close - plain config.mak.

I set $prefix there and ran "make install-doc". That installed the man pages in Documentation/share/man/ in my Git source directory.

Show 17 quoted lines
> I _think_ exporting mandir/html/infodir from the top-level Makefile
> is wrong to begin with.  We should drop the "export mandir" from
> there.
> 
> Giving them unusual meaning (e.g. "mandir = share/man") is already
> bad and that needs to be fixed by limiting this "oh, on some
> platforms we compile-in GIT_MAN_PATH as a relative path to an
> unspecified place" insanity only to where -DGIT_MAN_PATH=<path> is
> defined.  The path used there does not help the building and
> installation of the documentation at all, so the variable used for
> the purpose of giving that <path> should not be named the same way
> as the variable used on Documentation/Makefile to name the real path
> in the first place.
> 
> Perhaps rename these to runtime_{man,html,info}dir or something and
> make sure {man,html,info}dir are defined as the real paths whose
> default values begin with $(prefix)?

Would it be sensible to define the values for these variables (with absolute paths) in a separate top-level file like config.mak.uname (defaults.mak maybe?) and include that in both Documentation/Makefile and Makefile, then calculate the relative path from $(prefix) to $({man,html,info}dir) for the compiled-in values.

John

← back to recent threads