Fix installation paths with "make install-doc"

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

Signed-off-by John Keeping <[email protected]>

Documentation/Makefile +1 -0

@@ -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

Quoted 1 line · John Keeping

Signed-off-by John Keeping <[email protected]>

[...]

Quoted 14 lines

Documentation/Makefile +1 -0

@@ -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
 

Documentation/Makefile RM ?= rm -f · line 102

Quoted 2 lines

Documentation/Makefile +0 -1

-infodir ?= $(prefix)/share/info
 MAKEINFO = makeinfo

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

Documentation/Makefile: fix inherited {html,info,man}dir

Quoted 1 line · John Keeping

Documentation/Makefile: fix inherited {html,info,man}dir

This doesn't seem to have hit the list.

Quoted 5 lines · Jonathan Nieder

Quoted 1 line · John Keeping

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

Quoted 1 line · John Keeping

Documentation/Makefile: fix inherited {html,info,man}dir

This doesn't seem to have hit the list.

Quoted 4 lines

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.

Quoted 5 lines · Jonathan Nieder

Quoted 1 line · John Keeping

Documentation/Makefile: fix inherited {html,info,man}dir

This doesn't seem to have hit the list.

More importantly,

Quoted 4 lines

Quoted 4 lines

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)?

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

Quoted 3 lines · Junio C Hamano

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 <[email protected]>
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 <[email protected]>

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.

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.

Quoted 3 lines · Junio C Hamano

These are defined in the main Makefile to be funny values that are optionally relative to an unspecified location that is determined at runtime.

[...]

Quoted 4 lines

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 <[email protected]>

Reviewed-by Jonathan Nieder <[email protected]>

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.

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 <[email protected]>

Signed-off-by John Keeping <[email protected]>

---

Quoted 4 lines · Junio C Hamano

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(-)

Makefile +21 -15

index 7c75e3b..087eec4 100644
@@ -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 = \