Skip to contentSkip to conversation

RUNTIME_PREFIX relocatable Git

PATCH Superseded Dan Jacques 7 participants 34 messages Nov 29, 2017 – Apr 24, 2018 Kbuild

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Dan Jacques

author
#0 lore raw

v4 cover RUNTIME_PREFIX relocatable Git

Previous threads:
v1: https://public-inbox.org/git/[email protected]/
v2: https://public-inbox.org/git/[email protected]/
v3: https://public-inbox.org/git/[email protected]/

This is a small update to incorporate some Windows fixes from Johannes. At this point, it passes the full test suite on Linux, Mac, and FreeBSD, as well as the Travis.ci test suites, with and without RUNTIME_PREFIX/RUNTIME_PREFIX_PERL flags.

I'm happy with the patch set, and feel that it is ready to move forward. However, while it's been looked at by several people, and I have incorporated reviewer feedback, the patch set hasn't received any formal LGTM-style responses yet. I'm not sure what standard of review is required to move forward with a patch on this project, so maybe this is totally fine, but I wanted to make sure to point this out.

I also want to note Ævar Arnfjörð Bjarmason's RFC:
https://public-inbox.org/git/[email protected]/T/

The proposed patch set conflicts with the Perl installation directory changes in this patch set, as avarab@ notes. The proposed Perl installation process would simplify patch 0002 in this patch set. I don't think the landing order is terribly impactful - if this lands first, the patch in the RFC would delete a few more lines, and if this lands later, patch 0002 would largely not be necessary.

Cheers,
-Dan

Built using this "config.mak" w/ autoconf:

=== BEGIN config.mak ===

RUNTIME_PREFIX = YesPlease
RUNTIME_PREFIX_PERL = YesPlease
gitexecdir = libexec/git-core
template_dir = share/git-core/templates
sysconfdir = etc

=== END config.mak ===

Changes in v4 from v3:

- Incorporated some quoting and Makefile dependency fixes, courtesy of <[email protected]>.

Changes in v3 from v2:

- Broken into multiple patches now that Perl is isolated in its own RUNTIME_PREFIX_PERL flag.
- Working with avarab@, several changes to Perl script runtime prefix support:
- Moved Perl header body content from Makefile into external template

    file(s).

- Added generic "perllibdir" variable to override Perl installation path.
- RUNTIME_PREFIX_PERL generated script header is more descriptive and consistent with how the C version operates.
- Fixed Generated Perl header Makefile dependency, should rebuild when dependent files and flags change.
- Changed some of the new RUNTIME_PREFIX trace strings to use consistent formatting and terminology.

Changes in v2 from v1:

- Added comments and formatting to improve readability of platform-sepecific executable path resolution sleds in git_get_exec_path.
- Consolidated "cached_exec_path" and "argv_exec_path" globals into "exec_path_value".
- Use strbuf_realpath instead of realpath for procfs resolution.
- Removed new environment variable exports. Git with RUNTIME_PREFIX no longer exports or consumes any additional environment information.
- Updated Perl script resolution strategy: rather than having Git export the relative executable path to the Perl scripts, they now resolve it independently when RUNTIME_PREFIX_PERL is enabled.
- Updated resolution strategy for "gettext()": use system_path() instead of special environment variable.
- Added sysctl executable resolution support for BSDs that don't mount "procfs" by default (most of them).

Dan Jacques (4):
Makefile: generate Perl header from template file
Makefile: add support for "perllibdir"
Makefile: add Perl runtime prefix support
exec_cmd: RUNTIME_PREFIX on some POSIX systems

 .gitignore                             |   1 +
 Makefile                               | 111 +++++++++++++--
 cache.h                                |   1 +
 common-main.c                          |   4 +-
 config.mak.uname                       |   7 +
 exec_cmd.c                             | 239 ++++++++++++++++++++++++++++-----
 exec_cmd.h                             |   4 +-
 gettext.c                              |   8 +-
 git.c                                  |   2 +-
 perl/Makefile                          |  52 ++++++-
 perl/header_fixed_prefix.pl.template   |   1 +
 perl/header_runtime_prefix.pl.template |  24 ++++
 12 files changed, 396 insertions(+), 58 deletions(-)
 create mode 100644 perl/header_fixed_prefix.pl.template
 create mode 100644 perl/header_runtime_prefix.pl.template
Signature · 1 line
2.15.0.chromium12
author
#1 lore raw

PATCH v4 1/4 Makefile: generate Perl header from template file

Currently, the generated Perl script headers are emitted by commands in the Makefile. This mechanism restricts options to introduce alternative header content, needed by Perl runtime prefix support, and obscures the origin of the Perl script header.

Change the Makefile to generate a header by processing a template file and move the header content into the "perl/" subdirectory. The processed generated will now be stored in the "GIT-PERL-HEADER" file. This allows the content of the Perl header to be controlled by changing the path of the template in the Makefile.

Signed-off-by Dan Jacques <[email protected]>

Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
Thanks-to: Johannes Schindelin <[email protected]>

---
 .gitignore                           |  1 +
 Makefile                             | 24 +++++++++++++++---------
 perl/header_fixed_prefix.pl.template |  1 +
 3 files changed, 17 insertions(+), 9 deletions(-)
 create mode 100644 perl/header_fixed_prefix.pl.template
.gitignore +1 -0
index 833ef3b0b..89bd7bd8a 100644
@@ -3,6 +3,7 @@
 /GIT-LDFLAGS
 /GIT-PREFIX
 /GIT-PERL-DEFINES
+/GIT-PERL-HEADER
 /GIT-PYTHON-VARS
 /GIT-SCRIPT-DEFINES
 /GIT-USER-AGENT
Makefile +15 -9
index e53750ca0..f7c4ac207 100644
@@ -1964,18 +1964,15 @@ perl/PM.stamp: FORCE
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
+PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
-$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
+
+$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
-	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
-	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e '1{' \
 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
-	    -e '	h' \
-	    -e '	s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
-	    -e '	H' \
-	    -e '	x' \
+	    -e '	rGIT-PERL-HEADER' \
+	    -e '	G' \
 	    -e '}' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    $< >$@+ && \

@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
+	$(QUIET_GEN)$(RM) $@ && \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
+	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
+	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    $< >$@+ && \
+	mv $@+ $@
 
 .PHONY: gitweb
 gitweb:

@@ -2707,7 +2713,7 @@ ifndef NO_TCLTK
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
 	$(RM) GIT-USER-AGENT GIT-PREFIX
-	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PYTHON-VARS
+	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
perl/header_fixed_prefix.pl.template new file +1 -0
new file mode 100644
index 000000000..9a4bc4d30
@@ -0,0 +1 @@
+use lib (split(/@@PATHSEP@@/, $ENV{GITPERLLIB} || "@@INSTLIBDIR@@"));
Signature · 1 line
2.15.0.chromium12
#2 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Am 29.11.2017 um 16:56 schrieb Dan Jacques:

Makefile GIT-PERL-DEFINES: FORCE · line 1994

Quoted 10 lines
Makefile +6 -0
@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
  		echo "$$FLAGS" >$@; \
  	    fi
 
+GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
+	$(QUIET_GEN)$(RM) $@ && \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
+	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

Makefile GIT-PERL-DEFINES: FORCE · line 2000

Quoted 6 lines
Makefile +3 -0
+	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    $< >$@+ && \
+	mv $@+ $@
 
  .PHONY: gitweb
  gitweb:

-- Hannes

#3 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Hi Hannes,

Quoted 16 lines · Johannes Sixt

Am 29.11.2017 um 16:56 schrieb Dan Jacques:

Makefile GIT-PERL-DEFINES: FORCE · line 1994

Quoted 12 lines
Makefile +0 -0
@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
       	echo "$$FLAGS" >$@; \
       fi
  +GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak

Makefile

(quoted diff) +2 -0
+	$(QUIET_GEN)$(RM) $@ && \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory

instlibdir` && \

(quoted diff) +2 -0
+	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" &&

\

Makefile +1 -0
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

Funny, I tried this also with ';' as pathsep, and it worked in the Git for Windows SDK...

Ciao,
Dscho

#4 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Am 01.12.2017 um 18:13 schrieb Johannes Schindelin:

Quoted 21 lines

Hi Hannes,

Quoted 16 lines · Johannes Sixt

Am 29.11.2017 um 16:56 schrieb Dan Jacques:

Makefile GIT-PERL-DEFINES: FORCE · line 1994

Quoted 12 lines
Makefile +0 -0
@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
        	echo "$$FLAGS" >$@; \
        fi
   +GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak

Makefile

(quoted diff) +2 -0
+	$(QUIET_GEN)$(RM) $@ && \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory

instlibdir` && \

(quoted diff) +2 -0
+	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" &&

\

Makefile +1 -0
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

Quoted 2 lines

Funny, I tried this also with ';' as pathsep, and it worked in the Git for Windows SDK...

My ancient sed vs. your modern sed, perhaps? I can check this on Monday.

-- Hannes

author
#5 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 7 lines · Johannes Sixt
Quoted 5 lines
Quoted 2 lines

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

Funny, I tried this also with ';' as pathsep, and it worked in the Git for Windows SDK...

My ancient sed vs. your modern sed, perhaps? I can check this on Monday.

If you wouldn't mind, that would be much appreciated. Did you actually observe this issue, or is it just coming up in review?

I am not a sed wizard, but perhaps the tool is ignoring the semicolon because it's in the middle of the "s" expression? The shell may similarly be ignoring it because it's nested in between single-quotes?

-Dan

#6 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 3 lines · Dan Jacques

I am not a sed wizard, but perhaps the tool is ignoring the semicolon because it's in the middle of the "s" expression? The shell may similarly be ignoring it because it's nested in between single-quotes?

As far as POSIX is concerned, semicolons are not special, but can be used to separate commands instead of newlines.

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#7 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

In reply to Johannes Sixt

Am 01.12.2017 um 18:25 schrieb Johannes Sixt:

Quoted 26 lines

Am 01.12.2017 um 18:13 schrieb Johannes Schindelin:

Quoted 25 lines

Hi Hannes,

Quoted 20 lines · Johannes Sixt

Am 29.11.2017 um 16:56 schrieb Dan Jacques:

Quoted 6 lines
(quoted diff) +0 -0
@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
             echo "$$FLAGS" >$@; \

        fi
   +GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak
Makefile

Quoted 8 lines

+    $(QUIET_GEN)$(RM) $@ && \
+    INSTLIBDIR=MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir && \
+    INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+ INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+    sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete
sed expression because ';' is also a command separator in the sed language.

Quoted 5 lines
Quoted 3 lines

Funny, I tried this also with ';' as pathsep, and it worked in the Git for
Windows SDK...

My ancient sed vs. your modern sed, perhaps? I can check this on Monday.

I don't know what I tested last week; most likely not the version of the patch I quoted above.

Today's version, with the tip at 5d7f59c391ce, is definitely bogus with its quoting. It needs the patch below, otherwise an unquoted semicolon may be expanded from $(pathsep). This would terminate the sed command, of course.

Makefile +4 -4
index 484dc44ade..a658c8169a 100644
@@ -2071,10 +2071,10 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
-	sed -e 's=@@PATHSEP@@='$(pathsep)'=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-	    -e 's=@@GITEXECDIR@@='$(gitexecdir_relative_SQ)'=g' \
-	    -e 's=@@PERLLIBDIR@@='$(perllibdir_relative_SQ)'=g' \
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
+	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
+	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
 	    $< >$@
 
 .PHONY: gitweb
Signature · 1 line
2.14.2.808.g3bc32f2729
#8 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 4 lines · Johannes Sixt

Today's version, with the tip at 5d7f59c391ce, is definitely bogus with its quoting. It needs the patch below, otherwise an unquoted semicolon may be expanded from $(pathsep). This would terminate the sed command, of course.

Of course ;-)

Somehow I was lucky that the topic as been de-queued from 'pu' for the past few days.

Quoted 19 lines
Makefile +4 -4
index 484dc44ade..a658c8169a 100644
@@ -2071,10 +2071,10 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
-	sed -e 's=@@PATHSEP@@='$(pathsep)'=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-	    -e 's=@@GITEXECDIR@@='$(gitexecdir_relative_SQ)'=g' \
-	    -e 's=@@PERLLIBDIR@@='$(perllibdir_relative_SQ)'=g' \
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
+	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
+	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
 	    $< >$@
 
 .PHONY: gitweb
author
#9 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

In reply to Johannes Sixt
Quoted 7 lines · Johannes Sixt

I don't know what I tested last week; most likely not the version of the patch I quoted above.

Today's version, with the tip at 5d7f59c391ce, is definitely bogus with its quoting. It needs the patch below, otherwise an unquoted semicolon may be expanded from $(pathsep). This would terminate the sed command, of course.

Thanks for checking! The patch that you quoted above looks like it's from this "v4" thread; however, the patch that you are diffing against in your latest reply seems like it is from an earlier version.

I believe that the $(pathsep) changes in your proposed patch are already present in v4, having been caught by Johannes, so perhaps you were using an older version of the patch as the diffbase? That change only appeared in v4, so any older version would have had the previous incorrect quoting.

I think the reason that is necessary to remove the single quotes is not because the ";" is terminating the sed expression, but because it's terminating the entire shell stanza, causing the shell to execute two commands:

1) {"sed" "-e" "s=@@PATHSEP@@="}
2) {"=g" "-e" "s=@@INSTLIBDIR@@=..." ...}

By including the ";" within the single-tick-protected sed argument, the shell receives the entire block, "s=@@PATHSEP@@=;=", as a single string.

Also see Johannes' explanation here:
https://github.com/danjacques/git/pull/1/commits/57dc265478692ea2a395fc61fa122cb73ce58dc3

Could you apply this v4 patch series and confirm that it is working for you? Or, if I am mistaken and your patch is correctly against this "v4" patch series, let me know and I'll try and figure out what I'm missing.

Thanks again!
-Dan

#10 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 6 lines · Dan Jacques

Thanks for checking! The patch that you quoted above looks like it's from this "v4" thread; however, the patch that you are diffing against in your latest reply seems like it is from an earlier version.

I believe that the $(pathsep) changes in your proposed patch are already present in v4,...

You're of course right. The patches I had in my tree are outdated.

Will replace, even though I won't be merging them to 'pu' while we wait for Ævar's perl build procedure update to stabilize.

Thanks.

#11 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Am 05.12.2017 um 22:35 schrieb Junio C Hamano:

Quoted 6 lines
Quoted 4 lines · Dan Jacques

Thanks for checking! The patch that you quoted above looks like it's from this "v4" thread; however, the patch that you are diffing against in your latest reply seems like it is from an earlier version.

Quoted 7 lines
Quoted 2 lines

I believe that the $(pathsep) changes in your proposed patch are already present in v4,...

You're of course right. The patches I had in my tree are outdated.

Will replace, even though I won't be merging them to 'pu' while we wait for Ævar's perl build procedure update to stabilize.

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Makefile +1 -1
index 7ac4458f11..08c78a1a63 100644
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
 	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
 	    $< >$@+ && \
#12 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 3 lines · Johannes Sixt

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Yeah, that is very sensible.

Quoted 13 lines
Makefile +1 -1
index 7ac4458f11..08c78a1a63 100644
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
 	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
 	    $< >$@+ && \
author
#13 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Yeah, that is very sensible.

Quoted 13 lines

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile · line 2078

Quoted 13 lines
Makefile +1 -1
index 7ac4458f11..08c78a1a63 100644
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
      INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
      INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
      sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-         -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+         -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
          -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
          -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
          $< >$@+ && \

Sounds good; I'll apply that to my working patch and include it in my next ("v5") submission, which is currently blocked pending avarab@'s Perl Makefile changes:
https://public-inbox.org/git/[email protected]/T/#t

#14 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

Quoted 20 lines · Daniel Jacques
Quoted 19 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Yeah, that is very sensible.

Quoted 10 lines
Makefile +1 -1
index 7ac4458f11..08c78a1a63 100644
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
      INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
      INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
      sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-         -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+         -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
Quoted 8 lines
Quoted 3 lines

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile · line 2078

Quoted 3 lines
Makefile +0 -0
          -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
          -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
          $< >$@+ && \

Sounds good; I'll apply that to my working patch and include it in my next ("v5") submission, which is currently blocked pending avarab@'s Perl Makefile changes:
https://public-inbox.org/git/[email protected]/T/#t

Thanks, FWIW I'll send another version of that at the end of the week or so, I'm waiting to see if there's any more comments on it to reduce list churn.

#15 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

On Wed, Dec 06 2017, Ævar Arnfjörð Bjarmason jotted:

Quoted 30 lines · Daniel Jacques
Quoted 20 lines · Junio C Hamano
Quoted 19 lines
Quoted 3 lines · Johannes Sixt

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Yeah, that is very sensible.

Quoted 10 lines
Makefile +1 -1
index 7ac4458f11..08c78a1a63 100644
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
      INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
      INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
      sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-         -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+         -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
Quoted 8 lines
Quoted 3 lines

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile · line 2078

Quoted 3 lines
Makefile +0 -0
          -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
          -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
          $< >$@+ && \

Sounds good; I'll apply that to my working patch and include it in my next ("v5") submission, which is currently blocked pending avarab@'s Perl Makefile changes:
https://public-inbox.org/git/[email protected]/T/#t

Quoted 3 lines

Thanks, FWIW I'll send another version of that at the end of the week or so, I'm waiting to see if there's any more comments on it to reduce list churn.

Sorry, I got this conflated with my sha1collisiondetection series, I have nothing new to send out.

It seems everyone's happy with the version of my v2 (with Junio's <[email protected]> on top) from this series, so it's just a matter of waiting.

#16 lore raw

cover Handle $IFS in $INSTLIBDIR

In reply to Johannes Sixt

Hi,

Quoted 24 lines · Johannes Sixt

Am 05.12.2017 um 22:35 schrieb Junio C Hamano:

Quoted 6 lines
Quoted 4 lines · Dan Jacques

Thanks for checking! The patch that you quoted above looks like it's from this "v4" thread; however, the patch that you are diffing against in your latest reply seems like it is from an earlier version.

Quoted 7 lines
Quoted 2 lines

I believe that the $(pathsep) changes in your proposed patch are already present in v4,...

You're of course right. The patches I had in my tree are outdated.

Will replace, even though I won't be merging them to 'pu' while we wait for Ævar's perl build procedure update to stabilize.

The updated series works for me now. Nevertheless, I suggest to squash in the following change to protect against IFS and globbing characters in $INSTLIBDIR.

Makefile +0 -0
index 7ac4458f11..08c78a1a63 100644

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile · line 2077

Quoted 7 lines
Makefile +1 -1
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
 	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \

I just ran into this. Here's a pair of patches to fix it.

Thanks,
Jonathan Nieder (2):
Makefile: remove unused substitution variable @@PERLLIBDIR@@
Makefile: quote $INSTLIBDIR when passing it to sed

 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Signature · 1 line
2.17.0.441.gb46fe60e1d
#17 lore raw

PATCH 1/2 Makefile: remove unused @@PERLLIBDIR@@ substitution variable

Junio noticed that this variable is not quoted correctly when it is passed to sed. As a shell-quoted string, it should be inside single-quotes like $(perllibdir_relative_SQ), not outside them like $INSTLIBDIR.

In fact, this substitution variable is not used. Simplify by removing it.

Reported-by Junio C Hamano <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---
An unrelated cleanup noticed while looking over this code.

 Makefile | 1 -
 1 file changed, 1 deletion(-)
Makefile +0 -1
index 154929f1c8..8f4cb506ff 100644
@@ -2109,7 +2109,6 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
 	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-	    -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
Signature · 1 line
2.17.0.441.gb46fe60e1d
#18 lore raw

Re: [PATCH 1/2] Makefile: remove unused @@PERLLIBDIR@@ substitution variable

Quoted 4 lines · Jonathan Nieder

Junio noticed that this variable is not quoted correctly when it is passed to sed. As a shell-quoted string, it should be inside single-quotes like $(perllibdir_relative_SQ), not outside them like $INSTLIBDIR.

Spreading credit is very much appreciated, but in this case the above belongs below the three-dash fold, I would think, as the incorrect quoting become irrelevant.

Will queue. Thanks.

Quoted 23 lines

In fact, this substitution variable is not used. Simplify by removing it.

Reported-by Junio C Hamano <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---
An unrelated cleanup noticed while looking over this code.

 Makefile | 1 -
 1 file changed, 1 deletion(-)
Makefile +0 -1
index 154929f1c8..8f4cb506ff 100644
@@ -2109,7 +2109,6 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
 	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-	    -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
#19 lore raw

PATCH 2/2 Makefile: quote $INSTLIBDIR when passing it to sed

In reply to Jonathan Nieder

f6a0ad4b (Makefile: generate Perl header from template file, 2018-04-10) moved code for generating the 'use lib' lines at the top of perl scripts from the $(SCRIPT_PERL_GEN) rule to a separate GIT-PERL-HEADER rule.

This rule first populates INSTLIBDIR and then substitutes it into the GIT-PERL-HEADER using sed:

	INSTLIBDIR=... something ...
	sed -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' $< > $@

Because $INSTLIBDIR is not surrounded by double quotes, the shell splits it at each space, causing errors if INSTLIBDIR contains an $IFS character:

sed: 1: "s=@@INSTLIBDIR@@=/usr/l ...": unescaped newline inside substitute pattern

Add back the missing double-quotes to make it work again.

Signed-off-by Jonathan Nieder <[email protected]>

---
Thanks for reading.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index 8f4cb506ff..727eca5d0a 100644
@@ -2108,7 +2108,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
 	    -e 's=@@LOCALEDIR_REL@@=$(localedir_relative_SQ)=g' \
Signature · 1 line
2.17.0.441.gb46fe60e1d
#20 lore raw

Re: [PATCH 2/2] Makefile: quote $INSTLIBDIR when passing it to sed

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile · line 2111

Quoted 10 lines · Jonathan Nieder
Makefile +1 -1
index 8f4cb506ff..727eca5d0a 100644
@@ -2108,7 +2108,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \

Good find. FWIW, I'd find it a lot easier to read if the whole thing were enclosed inside a single pair of dq.

#21 lore raw

PATCH v2 2/2 Makefile: quote $INSTLIBDIR when passing it to sed

f6a0ad4b (Makefile: generate Perl header from template file, 2018-04-10) moved some code for generating the 'use lib' lines at the top of perl scripts from the $(SCRIPT_PERL_GEN) rule to a separate GIT-PERL-HEADER rule.

This rule first populates INSTLIBDIR and then substitutes it into the GIT-PERL-HEADER using sed:

	INSTLIBDIR=... something ...
	sed -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' $< > $@

Because $INSTLIBDIR is not surrounded by double quotes, the shell splits it at each space, causing errors if INSTLIBDIR contains a space:

sed: 1: "s=@@INSTLIBDIR@@=/usr/l ...": unescaped newline inside substitute pattern

Add back the missing double-quotes to make it work again.

Improved-by Junio C Hamano <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---
Hi,

Quoted 12 lines · Junio C Hamano

Review GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile · line 2111

Quoted 7 lines · Jonathan Nieder

+++ b/Makefile

(quoted diff) +1 -1
@@ -2108,7 +2108,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \

Good find. FWIW, I'd find it a lot easier to read if the whole thing were enclosed inside a single pair of dq.

Thanks. I agree, so here's an updated version doing that.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index 2327ccb906..5e25441861 100644
@@ -2116,7 +2116,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
 	    -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
 	    -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
 	    -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
Signature · 1 line
2.17.0.441.gb46fe60e1d
author
#22 lore raw

Re: [PATCH 2/2 v2] Makefile: quote $INSTLIBDIR when passing it to sed

Good catch, thanks for doing this!
-Dan

Quoted 8 lines · Jonathan Nieder

f6a0ad4b (Makefile: generate Perl header from template file, 2018-04-10) moved some code for generating the 'use lib' lines at the top of perl scripts from the $(SCRIPT_PERL_GEN) rule to a separate GIT-PERL-HEADER rule.

This rule first populates INSTLIBDIR and then substitutes it into the GIT-PERL-HEADER using sed:

Quoted 8 lines

INSTLIBDIR=... something ...
sed -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' $< > $@

Because $INSTLIBDIR is not surrounded by double quotes, the shell splits it at each space, causing errors if INSTLIBDIR contains a space:

sed: 1: "s=@@INSTLIBDIR@@=/usr/l ...": unescaped newline inside

substitute pattern

Quoted 4 lines

Add back the missing double-quotes to make it work again.

Improved-by Junio C Hamano <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>
Quoted 8 lines

---
Hi,

Quoted 4 lines · Junio C Hamano

Review GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)

Quoted 2 lines · Jonathan Nieder

+++ b/Makefile

(quoted diff) +0 -0
@@ -2108,7 +2108,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)

GIT-PERL-DEFINES Makefile

Quoted 2 lines
Quoted 2 lines

Review GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) · line 2109

Quoted 2 lines
(quoted diff) +0 -0
     INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \

INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)

Quoted 17 lines
Quoted 6 lines

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile

Quoted 3 lines
Makefile +1 -1
     sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-        -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+        -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \

Good find. FWIW, I'd find it a lot easier to read if the whole thing were enclosed inside a single pair of dq.

Thanks. I agree, so here's an updated version doing that.

  Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +0 -0
index 2327ccb906..5e25441861 100644
@@ -2116,7 +2116,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)

GIT-PERL-DEFINES Makefile

Makefile GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile · line 2116

Quoted 1 line
Makefile +0 -0
         INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \

INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \

Quoted 8 lines
Makefile +1 -2
         sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-           -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+           -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
             -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
             -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
             -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
--

2.17.0.441.gb46fe60e1d

#23 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

In reply to Johannes Sixt
Quoted 5 lines · Johannes Sixt

Makefile GIT-PERL-DEFINES: FORCE · line 1994

Quoted 1 line
Makefile +1 -0
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

It is correct that ';' can be and does get used in place of LF when writing a script on a single line, but even then, as part of a string argument to 's' command (and also others), there is no need to quote ';' or otherwise treat it any specially, as the commands know what their syntax is (e.g. 's=string=replacement=' after seeing the first '=' knows that it needs to find one unquoted '=' to find the end of the first argument, and another to find the end of the replacement string, and ';' seen during that scanning would not have any special meaning).

If your sed is so broken and does not satisfy the above expectation, t6023 would not work for you, I would gess.

t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" < new5.txt > new6.txt t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" < new5.txt > new7.txt t/t6023-merge-file.sh:sed -e 's/deerit./&%%%%/' -e "s/locavit,/locavit;/"< new6.txt | tr '%' '\012' > new8.txt

#24 lore raw

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

On Sun, Dec 03 2017, Junio C. Hamano jotted:

Quoted 18 lines
Quoted 5 lines · Johannes Sixt

Makefile GIT-PERL-DEFINES: FORCE · line 1994

Quoted 1 line
Makefile +1 -0
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \

This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete sed expression because ';' is also a command separator in the sed language.

It is correct that ';' can be and does get used in place of LF when writing a script on a single line, but even then, as part of a string argument to 's' command (and also others), there is no need to quote ';' or otherwise treat it any specially, as the commands know what their syntax is (e.g. 's=string=replacement=' after seeing the first '=' knows that it needs to find one unquoted '=' to find the end of the first argument, and another to find the end of the replacement string, and ';' seen during that scanning would not have any special meaning).

Quoted 6 lines

If your sed is so broken and does not satisfy the above expectation, t6023 would not work for you, I would gess.

t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" < new5.txt > new6.txt t/t6023-merge-file.sh:sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" < new5.txt > new7.txt t/t6023-merge-file.sh:sed -e 's/deerit./&%%%%/' -e "s/locavit,/locavit;/"< new6.txt | tr '%' '\012' > new8.txt

Since this whole thing is guarded by "ifndef NO_PERL" Dan could just be using "perl -pe" here instead of fiddling around with the portability edge cases of sed, e.g.:

    perl -pe 's[foo][bar[g' <in >out
author
#25 lore raw

PATCH v4 2/4 Makefile: add support for "perllibdir"

In reply to Dan Jacques

Add the "perllibdir" Makefile variable, which allows the customization of the Perl library installation path.

The Perl library installation path is currently left entirely to the Perl Makefile implementation, either MakeMaker (default) or a fixed path when NO_PERL_MAKEMAKER is enabled. This patch introduces "perllibdir", a Makefile variable that can override that Perl module installation path.

As with some other Makefile variables, "perllibdir" may be either absolute or relative. In the latter case, it is treated as relative to "$(prefix)".

Add some incidental documentation to perl/Makefile.

Explicitly specifying an installation path is necessary for Perl runtime prefix support, as runtime prefix resolution code must know in advance where the Perl support modules are installed.

Signed-off-by Dan Jacques <[email protected]>
---
 Makefile      | 18 +++++++++++++-----
 perl/Makefile | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 59 insertions(+), 11 deletions(-)
Makefile +13 -5
index f7c4ac207..80904f8b0 100644
@@ -462,6 +462,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   perllibdir
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)

@@ -1721,6 +1722,7 @@ gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
+perllibdir_SQ = $(subst ','\'',$(perllibdir))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

@@ -1955,17 +1957,22 @@ $(SCRIPT_PERL_GEN): perl/perl.mak
 
 perl/perl.mak: perl/PM.stamp
 
-perl/PM.stamp: FORCE
+perl/PM.stamp: GIT-PERL-DEFINES FORCE
 	@$(FIND) perl -type f -name '*.pm' | sort >$@+ && \
+	cat GIT-PERL-DEFINES >>$@+ && \
 	$(PERL_PATH) -V >>$@+ && \
 	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
 	$(RM) $@+
 
-perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
-	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
-
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
+
+PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER)
+PERL_DEFINES += $(perllibdir)
+
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
+	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' \
+	  prefix='$(prefix_SQ)' perllibdir='$(perllibdir_SQ)' $(@F)
 
 $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \

@@ -1979,6 +1986,7 @@ $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GI
 	chmod +x $@+ && \
 	mv $@+ $@
 
+PERL_DEFINES := $(subst $(space),:,$(PERL_DEFINES))
 GIT-PERL-DEFINES: FORCE
 	@FLAGS='$(PERL_DEFINES)'; \
 	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
perl/Makefile +46 -6

84 lines not on this page · raw message

Signature · 1 line
2.15.0.chromium12
#26 lore raw

Re: [PATCH v4 2/4] Makefile: add support for "perllibdir"

On Wed, Nov 29 2017, Dan Jacques jotted:

Quoted 110 lines

Add the "perllibdir" Makefile variable, which allows the customization of the Perl library installation path.

The Perl library installation path is currently left entirely to the Perl Makefile implementation, either MakeMaker (default) or a fixed path when NO_PERL_MAKEMAKER is enabled. This patch introduces "perllibdir", a Makefile variable that can override that Perl module installation path.

As with some other Makefile variables, "perllibdir" may be either absolute or relative. In the latter case, it is treated as relative to "$(prefix)".

Add some incidental documentation to perl/Makefile.

Explicitly specifying an installation path is necessary for Perl runtime prefix support, as runtime prefix resolution code must know in advance where the Perl support modules are installed.

Signed-off-by Dan Jacques <[email protected]>
---
 Makefile      | 18 +++++++++++++-----
 perl/Makefile | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 59 insertions(+), 11 deletions(-)
Makefile +13 -5
index f7c4ac207..80904f8b0 100644
@@ -462,6 +462,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   perllibdir
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)

@@ -1721,6 +1722,7 @@ gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
+perllibdir_SQ = $(subst ','\'',$(perllibdir))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

@@ -1955,17 +1957,22 @@ $(SCRIPT_PERL_GEN): perl/perl.mak
 
 perl/perl.mak: perl/PM.stamp
 
-perl/PM.stamp: FORCE
+perl/PM.stamp: GIT-PERL-DEFINES FORCE
 	@$(FIND) perl -type f -name '*.pm' | sort >$@+ && \
+	cat GIT-PERL-DEFINES >>$@+ && \
 	$(PERL_PATH) -V >>$@+ && \
 	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
 	$(RM) $@+
 
-perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
-	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
-
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
+
+PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER)
+PERL_DEFINES += $(perllibdir)
+
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
+	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' \
+	  prefix='$(prefix_SQ)' perllibdir='$(perllibdir_SQ)' $(@F)
 
 $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \

@@ -1979,6 +1986,7 @@ $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GI
 	chmod +x $@+ && \
 	mv $@+ $@
 
+PERL_DEFINES := $(subst $(space),:,$(PERL_DEFINES))
 GIT-PERL-DEFINES: FORCE
 	@FLAGS='$(PERL_DEFINES)'; \
 	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
perl/Makefile +16 -0
index f657de20e..b2aeeb0d8 100644
@@ -1,6 +1,22 @@
 #
 # Makefile for perl support modules and routine
 #
+# This Makefile generates "perl.mak", which contains the actual build and
+# installation directions.
+#
+# PERL_PATH must be defined to be the path of the Perl interpreter to use.
+#
+# prefix must be defined as the Git installation prefix.
+#
+# localedir must be defined as the path to the locale data.
+#
+# perllibdir may be optionally defined to override the default Perl module
+# installation directory, which is relative to prefix. If perllibdir is not
+# absolute, it will be treated as relative to prefix.
+#
+# NO_PERL_MAKEMAKER may be defined to use a built-in Makefile generation method
+# instead of Perl MakeMaker.
+
 makfile:=perl.mak
 modules =
 

@@ -12,6 +28,16 @@ ifndef V
 	QUIET = @
 endif

perl/Makefile ifndef V · line 39

Quoted 10 lines
perl/Makefile +9 -0
 
+# If a library directory is provided, and it is not an absolute path, resolve
+# it relative to prefix.
+ifneq ($(perllibdir),)
+ifneq ($(filter /%,$(firstword $(perllibdir))),)
+perllib_instdir = $(perllibdir)
+else
+perllib_instdir = $(prefix)/$(perllibdir)
+endif
+endif

Maybe I'm missing something, but isn't this "perllibdir can be relative" aim orthagonal to what this patch series is about? I.e. we could just avoid this work by saying you must say "prefix=/usr perllibdir=/usr/perl" instead of "prefix=/usr perllibdir=perl" as this allows.

I started going down this route with my own patch and just threw it away.

Quoted 52 lines
perl/Makefile +20 -6
 all install instlibdir: $(makfile)
 	$(QUIET)$(MAKE) -f $(makfile) $@
 

@@ -25,7 +51,12 @@ clean:
 $(makfile): PM.stamp
 
 ifdef NO_PERL_MAKEMAKER
-instdir_SQ = $(subst ','\'',$(prefix)/lib)
+
+ifeq ($(perllib_instdir),)
+perllib_instdir = $(prefix)/lib
+endif
+
+instdir_SQ = $(subst ','\'',$(perllib_instdir))
 
 modules += Git
 modules += Git/I18N

@@ -42,7 +73,7 @@ modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
 modules += Git/SVN/Utils
 
-$(makfile): ../GIT-CFLAGS Makefile
+$(makfile): ../GIT-CFLAGS ../GIT-PERL-DEFINES Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
 	set -e; \
 	for i in $(modules); \

@@ -79,12 +110,21 @@ $(makfile): ../GIT-CFLAGS Makefile
 	echo '	cp private-Error.pm "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
 	echo instlibdir: >> $@
 	echo '	echo $(instdir_SQ)' >> $@
+
 else
-$(makfile): Makefile.PL ../GIT-CFLAGS
-	$(PERL_PATH) $< PREFIX='$(prefix_SQ)' INSTALL_BASE='' --localedir='$(localedir_SQ)'
+
+# This may be empty if perllibdir was empty.
+instdir_SQ = $(subst ','\'',$(perllib_instdir))
+
+$(makfile): Makefile.PL ../GIT-CFLAGS ../GIT-PERL-DEFINES
+	$(PERL_PATH) $< \
+	  PREFIX='$(prefix_SQ)' INSTALL_BASE='' \
+	  LIB='$(instdir_SQ)' \
+	  --localedir='$(localedir_SQ)'
+
 endif
 
 # this is just added comfort for calling make directly in perl dir
 # (even though GIT-CFLAGS aren't used yet. If ever)
-../GIT-CFLAGS:
-	$(MAKE) -C .. GIT-CFLAGS
+../GIT-CFLAGS ../GIT-PERL-DEFINES:
+	$(MAKE) -C .. $(@F)
author
#27 lore raw

PATCH v4 3/4 Makefile: add Perl runtime prefix support

In reply to Dan Jacques

Add a new Makefile flag, RUNTIME_PREFIX_PERL, which, when enabled, configures Perl scripts to locate the Git installation's Perl support libraries by resolving against the script's path, rather than hard-coding that path at build-time.

Enabling RUNTIME_PREFIX_PERL overrides the system-specific Perl script installation path generated by MakeMaker to force installation into a platform-neutral location, "<prefix>/share/perl5".

This change enables Git's Perl scripts to work when their Git installation is relocated or moved to another system.

Signed-off-by Dan Jacques <[email protected]>

Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
Thanks-to: Johannes Schindelin <[email protected]>

---
 Makefile                               | 40 +++++++++++++++++++++++++++++++++-
 perl/header_runtime_prefix.pl.template | 24 ++++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 perl/header_runtime_prefix.pl.template
Makefile +39 -1
index 80904f8b0..741d1583f 100644
@@ -425,6 +425,10 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define RUNTIME_PREFIX_PERL to configure Git's PERL commands to locate Git
+# support libraries relative to their filesystem path instead of hard-coding
+# it.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -485,6 +489,7 @@ pathsep = :
 
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+sharedir_relative = $(patsubst $(prefix)/%,%,$(sharedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir

@@ -1967,7 +1972,38 @@ perl/PM.stamp: GIT-PERL-DEFINES FORCE
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
 
 PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
-PERL_DEFINES += $(NO_PERL_MAKEMAKER)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER) $(RUNTIME_PREFIX_PERL)
+
+# Support Perl runtime prefix. In this mode, a different header is installed
+# into Perl scripts. This header expects both the scripts and their support
+# library to be installed relative to $(prefix), and resolves the path to
+# the Perl libraries (perllibdir) from the executable's current path
+# (gitexecdir).
+#
+# This configuration requires both $(perllibdir) and $(gitexecdir) to be
+# relative paths, and will error if this is not the case.
+ifdef RUNTIME_PREFIX_PERL
+
+PERL_HEADER_TEMPLATE = perl/header_runtime_prefix.pl.template
+PERL_DEFINES += $(gitexecdir)
+
+# RUNTIME_PREFIX_PERL requires a $(perllibdir) value.
+ifeq ($(perllibdir),)
+perllibdir = $(sharedir_relative)/perl5
+endif
+
+ifneq ($(filter /%,$(firstword $(gitexecdir))),)
+$(error RUNTIME_PREFIX_PERL requires a relative gitexecdir, not: $(gitexecdir))
+endif
+gitexecdir_relative_SQ = $(gitexecdir_SQ)
+
+ifneq ($(filter /%,$(firstword $(perllibdir))),)
+$(error RUNTIME_PREFIX_PERL requires a relative perllibdir, not: $(perllibdir))
+endif
+perllibdir_relative_SQ = $(perllibdir_SQ)
+
+endif
+
 PERL_DEFINES += $(perllibdir)
 
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL

@@ -2001,6 +2037,8 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
 	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
+	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
 	    $< >$@+ && \
 	mv $@+ $@
perl/header_runtime_prefix.pl.template new file +24 -0
new file mode 100644
index 000000000..fb9a9924d
@@ -0,0 +1,24 @@
+# BEGIN RUNTIME_PREFIX_PERL generated code.
+#
+# This finds our Git::* libraries relative to the script's runtime path.
+BEGIN {
+	use lib split /@@PATHSEP@@/,
+	(
+		$ENV{GITPERLLIB}
+		||
+		do {
+			require FindBin;
+			require File::Spec;
+			my $gitexecdir_relative = '@@GITEXECDIR@@';
+			my $perllibdir_relative = '@@PERLLIBDIR@@';
+
+			($FindBin::Bin =~ m=${gitexecdir_relative}$=) ||
+			    die('Unrecognized runtime path.');
+			my $prefix = substr($FindBin::Bin, 0, -length($gitexecdir_relative));
+			my $perllibdir = File::Spec->catdir($prefix, $perllibdir_relative);
+			(-e $perllibdir) || die("Invalid library path: $perllibdir");
+			$perllibdir;
+		}
+	);
+}
+# END RUNTIME_PREFIX_PERL generated code.
Signature · 1 line
2.15.0.chromium12
#28 lore raw

Re: [PATCH v4 3/4] Makefile: add Perl runtime prefix support

On Wed, Nov 29 2017, Dan Jacques jotted:

Quoted 25 lines

Add a new Makefile flag, RUNTIME_PREFIX_PERL, which, when enabled, configures Perl scripts to locate the Git installation's Perl support libraries by resolving against the script's path, rather than hard-coding that path at build-time.
[...]

perl/header_runtime_prefix.pl.template new file +14 -0
new file mode 100644
index 000000000..fb9a9924d
@@ -0,0 +1,24 @@
+# BEGIN RUNTIME_PREFIX_PERL generated code.
+#
+# This finds our Git::* libraries relative to the script's runtime path.
+BEGIN {
+	use lib split /@@PATHSEP@@/,
+	(
+		$ENV{GITPERLLIB}
+		||
+		do {
+			require FindBin;
+			require File::Spec;
+			my $gitexecdir_relative = '@@GITEXECDIR@@';
+			my $perllibdir_relative = '@@PERLLIBDIR@@';
+

perl/header_runtime_prefix.pl.template line 24

Quoted 10 lines
perl/header_runtime_prefix.pl.template new file +10 -0
+			($FindBin::Bin =~ m=${gitexecdir_relative}$=) ||
+			    die('Unrecognized runtime path.');
+			my $prefix = substr($FindBin::Bin, 0, -length($gitexecdir_relative));
+			my $perllibdir = File::Spec->catdir($prefix, $perllibdir_relative);
+			(-e $perllibdir) || die("Invalid library path: $perllibdir");
+			$perllibdir;
+		}
+	);
+}
+# END RUNTIME_PREFIX_PERL generated code.

Ah, I see. To answer my own question in <[email protected]> you're making this stuff a relative path so you can use it here later on. I.e. we $FindBin::Bin, and then go from there. Makes sense.

We could use $ENV{GIT_EXEC_PATH} instead of FindBin here though, I missed that the first time. But that's just a nano-optimization. I just wondered whether git wasn't already passing us this info.

There is one remaining bug here. Git::I18N isn't doing the right thing, I installed in /tmp/git and moved to /tmp/git2, and it has:

    our $TEXTDOMAINDIR = $ENV{GIT_TEXTDOMAINDIR} || '/tmp/git/share/locale';

And GIT_TEXTDOMAINDIR is not passed by git (it's only used for the tests IIRC). Would need a similar treatment as this. Easiest to just set the path we find here in $Git::Whatever and pick it up in $Git::I18N later, it's not like anyone uses it outside of git.git.

But that does raise a more general concern for me. Isn't there some way we can run the test suite against an installed git (don't remember), then build, install, move the dir, and run the tests from the moved dir.

That would have caught this bug, and anything else that may be lurking still.

author
#29 lore raw

My initial set of replies are inline here:
https://public-inbox.org/git/[email protected]/T/#m1ff5cda787eaea4a4015a8f00a8be5966122c73a

I put together this solution for the I18N module. It exports "localedir" to the Perl header and injects the correct value into Git::I18N. It also suppresses the emission of the full ++LOCALEDIR++ when in runtime prefix mode.

One downside of this approach is that, since the point of resolution happens at the beginning of the script execution, external users of the runtime prefix Git::I18N module will not be able to resolve the locale directory. I think this is okay, though, since RUNTIME_PREFIX_PERL is decidedly *not* intended for system installation and is not trying to offer system module import portability.

After looking a bit at your $ENV{GIT_EXEC_PATH} optimization suggestion, I ended up favoring the current usage of FindBin::Bin for a few reasons:

- Encoding the relative GITEXECDIR into the script is more consistent with how "exec_cmd.c" does its own self-resolution.
- The resolution code is a bit more readable, both in terms of what it's doing and where it starts from.
- It makes the scripts individually executable, rather than having to go through the Git wrapper. This is, minimally, useful for testing.

I admittedly don't know much about FindBin, so if it has a downside that could outweigh this I'm more than happy to reevaluate.

With respect to testing, I did find "GIT_TEST_INSTALLED". Unfortunately, it doesn't seem to work against the full test suite even on master, and individual tests, especially Perl and locale, set their own path expectations. I think including installation-relative testing here would take a fair amount of work.

Note that this is a replacement for [PATCH v4 3/4], so if you're trying to apply it, you will need to build it on top of the preceding patches.

Many thanks,
-Dan

---
 Makefile                               | 59 +++++++++++++++++++++++++++++++---
 perl/Git/I18N.pm                       |  2 +-
 perl/Makefile                          | 17 ++++++----
 perl/header_runtime_prefix.pl.template | 31 ++++++++++++++++++
 t/test-lib.sh                          | 13 +++++---
 5 files changed, 107 insertions(+), 15 deletions(-)
 create mode 100644 perl/header_runtime_prefix.pl.template
Makefile +55 -4
index 80904f8b0..558bd3ebb 100644
@@ -425,6 +425,10 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define RUNTIME_PREFIX_PERL to configure Git's PERL commands to locate Git
+# support libraries relative to their filesystem path instead of hard-coding
+# it.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -462,6 +466,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   localedir
 #   perllibdir
 # This can help installing the suite in a relocatable way.
 

@@ -483,9 +488,12 @@ lib = lib
 # DESTDIR =
 pathsep = :
 
+gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
+perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir
 

@@ -1718,11 +1726,13 @@ bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
 infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 localedir_SQ = $(subst ','\'',$(localedir))
+localedir_relative_SQ = $(subst ','\'',$(localedir_relative))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
+gitexecdir_relative_SQ = $(subst ','\'',$(gitexecdir_relative))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
-perllibdir_SQ = $(subst ','\'',$(perllibdir))
+perllibdir_relative_SQ = $(subst ','\'',$(perllibdir_relative))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

@@ -1965,14 +1975,52 @@ perl/PM.stamp: GIT-PERL-DEFINES FORCE
 	$(RM) $@+
 
 PERL_HEADER_TEMPLATE = perl/header_fixed_prefix.pl.template
+perl_localedir = $(localedir)
 
 PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ)
-PERL_DEFINES += $(NO_PERL_MAKEMAKER)
-PERL_DEFINES += $(perllibdir)
+PERL_DEFINES += $(NO_PERL_MAKEMAKER) $(RUNTIME_PREFIX_PERL)
+
+# Support Perl runtime prefix. In this mode, a different header is installed
+# into Perl scripts. This header expects both the scripts and their support
+# library to be installed relative to $(prefix), and resolves the path to
+# the Perl libraries (perllibdir) from the executable's current path
+# (gitexecdir).
+#
+# This configuration requires both $(perllibdir) and $(gitexecdir) to be
+# relative paths, and will error if this is not the case.
+ifdef RUNTIME_PREFIX_PERL
+
+PERL_HEADER_TEMPLATE = perl/header_runtime_prefix.pl.template
+
+# RUNTIME_PREFIX_PERL requires a $(perllibdir) value.
+ifeq ($(perllibdir),)
+perllibdir = $(sharedir)/perl5
+endif
+
+ifneq ($(filter /%,$(firstword $(perllibdir_relative))),)
+$(error RUNTIME_PREFIX_PERL requires a relative perllibdir, not: $(perllibdir))
+endif
+
+ifneq ($(filter /%,$(firstword $(gitexecdir_relative))),)
+$(error RUNTIME_PREFIX_PERL requires a relative gitexecdir, not: $(gitexecdir))
+endif
+
+ifneq ($(filter /%,$(firstword $(localedir_relative))),)
+$(error RUNTIME_PREFIX_PERL requires a prefix-relative localedir, not: $(localedir))
+endif
+
+# Don't need to export a locale directory to Perl scripts, since the runtime
+# header will resolve it.
+perl_localedir =
+
+endif
+
+PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
+export perl_localedir
 
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' \
-	  prefix='$(prefix_SQ)' perllibdir='$(perllibdir_SQ)' $(@F)
+	  prefix='$(prefix_SQ)' perllibdir='$(perllibdir_relative_SQ)' $(@F)
 
 $(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \

@@ -2001,6 +2049,9 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
 	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
+	    -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
+	    -e 's=@@LOCALEDIR@@=$(localedir_relative_SQ)=g' \
 	    $< >$@+ && \
 	mv $@+ $@
perl/Git/I18N.pm +1 -1
index 836a5c238..25141d27d 100644
@@ -18,7 +18,7 @@ our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
 	our $TEXTDOMAIN = 'git';
-	our $TEXTDOMAINDIR = $ENV{GIT_TEXTDOMAINDIR} || '++LOCALEDIR++';
+	our $TEXTDOMAINDIR ||= $ENV{GIT_TEXTDOMAINDIR} || '++LOCALEDIR++';
 
 	require POSIX;
 	POSIX->import(qw(setlocale));
perl/Makefile +11 -6
index b2aeeb0d8..14bdda99d 100644
@@ -8,7 +8,7 @@
 #
 # prefix must be defined as the Git installation prefix.
 #
-# localedir must be defined as the path to the locale data.
+# perl_localedir must be defined as the path to the locale data.
 #
 # perllibdir may be optionally defined to override the default Perl module
 # installation directory, which is relative to prefix. If perllibdir is not

@@ -22,7 +22,7 @@ modules =
 
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 prefix_SQ = $(subst ','\'',$(prefix))
-localedir_SQ = $(subst ','\'',$(localedir))
+localedir_SQ = $(subst ','\'',$(perl_localedir))
 
 ifndef V
 	QUIET = @

@@ -116,11 +116,16 @@ else
 # This may be empty if perllibdir was empty.
 instdir_SQ = $(subst ','\'',$(perllib_instdir))
 
+MAKEMAKER_PARAMS = PREFIX='$(prefix_SQ)' INSTALL_BASE=''
+ifneq ($(instdir_SQ),)
+MAKEMAKER_PARAMS += LIB='$(instdir_SQ)'
+endif
+ifneq ($(localedir_SQ),)
+MAKEMAKER_PARAMS += --localedir='$(localedir_SQ)'
+endif
+
 $(makfile): Makefile.PL ../GIT-CFLAGS ../GIT-PERL-DEFINES
-	$(PERL_PATH) $< \
-	  PREFIX='$(prefix_SQ)' INSTALL_BASE='' \
-	  LIB='$(instdir_SQ)' \
-	  --localedir='$(localedir_SQ)'
+	$(PERL_PATH) $< $(MAKEMAKER_PARAMS)
 
 endif
perl/header_runtime_prefix.pl.template new file +31 -0
new file mode 100644
index 000000000..9cc63cfb6
@@ -0,0 +1,31 @@
+# BEGIN RUNTIME_PREFIX_PERL generated code.
+#
+# This finds our Git::* libraries relative to the script's runtime path.
+sub __git_system_path {
+	my ($relpath) = @_;
+	my $gitexecdir_relative = '@@GITEXECDIR@@';
+
+	require FindBin;
+	require File::Spec;
+	($FindBin::Bin =~ m=${gitexecdir_relative}$=) || die('Unrecognized runtime path.');
+	my $prefix = substr($FindBin::Bin, 0, -length($gitexecdir_relative));
+	return File::Spec->catdir($prefix, $relpath);
+}
+
+BEGIN {
+	use lib split /@@PATHSEP@@/,
+	(
+		$ENV{GITPERLLIB} ||
+		do {
+			my $perllibdir = __git_system_path('@@PERLLIBDIR@@');
+			(-e $perllibdir) || die("Invalid system path ($relpath): $path");
+			$perllibdir;
+		}
+	);
+
+	# Export the system locale directory to the I18N module. The locale directory
+	# is only installed if NO_GETTEXT is set.
+	$Git::I18N::TEXTDOMAINDIR = __git_system_path('@@LOCALEDIR@@');
+}
+
+# END RUNTIME_PREFIX_PERL generated code.
t/test-lib.sh +9 -4
index 116bd6a70..d6ea423af 100644
@@ -919,11 +919,16 @@ then
 	fi
 fi
 
-GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
+if test -n "$GIT_TEST_PERLLIB"
+then
+  GITPERLLIB="$GIT_TEST_PERLLIB"
+else
+	GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
+	test -d "$GIT_BUILD_DIR"/templates/blt || {
+		error "You haven't built things yet, have you?"
+	}
+fi
 export GITPERLLIB
-test -d "$GIT_BUILD_DIR"/templates/blt || {
-	error "You haven't built things yet, have you?"
-}
 
 if ! test -x "$GIT_BUILD_DIR"/t/helper/test-chmtime
 then
Signature · 1 line
2.15.0.chromium12
#30 lore raw

Re: [PATCH v4 3/4] Makefile: add Perl runtime prefix support

In reply to Dan Jacques

On Wed, Nov 29 2017, Dan Jacques jotted:

Quoted 10 lines
perl/header_runtime_prefix.pl.template new file +10 -0
+	use lib split /@@PATHSEP@@/,
+	(
+		$ENV{GITPERLLIB}
+		||
+		do {
+			require FindBin;
+			require File::Spec;
+			my $gitexecdir_relative = '@@GITEXECDIR@@';
+			my $perllibdir_relative = '@@PERLLIBDIR@@';
+

perl/header_runtime_prefix.pl.template line 24

Quoted 10 lines
perl/header_runtime_prefix.pl.template new file +10 -0
+			($FindBin::Bin =~ m=${gitexecdir_relative}$=) ||
+			    die('Unrecognized runtime path.');
+			my $prefix = substr($FindBin::Bin, 0, -length($gitexecdir_relative));
+			my $perllibdir = File::Spec->catdir($prefix, $perllibdir_relative);
+			(-e $perllibdir) || die("Invalid library path: $perllibdir");
+			$perllibdir;
+		}
+	);
+}
+# END RUNTIME_PREFIX_PERL generated code.

Noticed after I sent the last E-Mail, this is missing @@INSTLIBDIR@@ which per my reading of it being initially introduced should be here in addition to this relative path.

My reading of the intial patch that added it, as indicated in my patch, is that it's the dir we're going to be installing our libs to, so I didn't fiddle with it, but I think with your patches we need that dir *and* or own $perllibdir.

author
#31 lore raw

PATCH v4 4/4 exec_cmd: RUNTIME_PREFIX on some POSIX systems

In reply to Dan Jacques

Enable Git to resolve its own binary location using a variety of OS-specific and generic methods, including:

- procfs via "/proc/self/exe" (Linux)
- _NSGetExecutablePath (Darwin)
- KERN_PROC_PATHNAME sysctl on BSDs.
- argv0, if absolute (all, including Windows).

This is used to enable RUNTIME_PREFIX support for non-Windows systems, notably Linux and Darwin. When configured with RUNTIME_PREFIX, Git will do a best-effort resolution of its executable path and automatically use this as its "exec_path" for relative helper and data lookups, unless explicitly overridden.

Small incidental formatting cleanup of "exec_cmd.c".

Signed-off-by Dan Jacques <[email protected]>

Thanks-to: Robbie Iannucci <[email protected]>
Thanks-to: Junio C Hamano <[email protected]>

---
 Makefile         |  35 +++++++-
 cache.h          |   1 +
 common-main.c    |   4 +-
 config.mak.uname |   7 ++
 exec_cmd.c       | 239 +++++++++++++++++++++++++++++++++++++++++++++++--------
 exec_cmd.h       |   4 +-
 gettext.c        |   8 +-
 git.c            |   2 +-
 8 files changed, 260 insertions(+), 40 deletions(-)
Makefile +34 -1
index 741d1583f..e663fda1f 100644
@@ -416,6 +416,16 @@ all::
 #
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
+# Define HAVE_BSD_KERN_PROC_SYSCTL if your platform supports the KERN_PROC BSD
+# sysctl function.
+#
+# Define PROCFS_EXECUTABLE_PATH if your platform mounts a "procfs" filesystem
+# capable of resolving the path of the current executable. If defined, this
+# must be the canonical path for the "procfs" current executable path.
+#
+# Define HAVE_NS_GET_EXECUTABLE_PATH if your platform supports calling
+# _NSGetExecutablePath to retrieve the path of the running executable.
+#
 # Define HAVE_GETDELIM if your system has the getdelim() function.
 #
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define

@@ -426,6 +436,12 @@ all::
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
 #
+# Define RUNTIME_PREFIX to configure Git to resolve its ancillary tooling and
+# support files relative to the location of the runtime binary, rather than
+# hard-coding them into the binary. Git installations built with RUNTIME_PREFIX
+# can be moved to arbitrary filesystem locations. Users may want to enable
+# RUNTIME_PREFIX_PERL as well (see below).
+#
 # Define RUNTIME_PREFIX_PERL to configure Git's PERL commands to locate Git
 # support libraries relative to their filesystem path instead of hard-coding
 # it.

@@ -466,6 +482,7 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   localedir
 #   perllibdir
 # This can help installing the suite in a relocatable way.
 

@@ -491,6 +508,7 @@ mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
 sharedir_relative = $(patsubst $(prefix)/%,%,$(sharedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
+localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir
 

@@ -1637,10 +1655,23 @@ ifdef HAVE_BSD_SYSCTL
 	BASIC_CFLAGS += -DHAVE_BSD_SYSCTL
 endif
 
+ifdef HAVE_BSD_KERN_PROC_SYSCTL
+	BASIC_CFLAGS += -DHAVE_BSD_KERN_PROC_SYSCTL
+endif
+
 ifdef HAVE_GETDELIM
 	BASIC_CFLAGS += -DHAVE_GETDELIM
 endif
 
+ifneq ($(PROCFS_EXECUTABLE_PATH),)
+	procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH))
+	BASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"'
+endif
+
+ifdef HAVE_NS_GET_EXECUTABLE_PATH
+	BASIC_CFLAGS += -DHAVE_NS_GET_EXECUTABLE_PATH
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

@@ -1723,6 +1754,7 @@ bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
 infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 localedir_SQ = $(subst ','\'',$(localedir))
+localedir_relative_SQ = $(subst ','\'',$(localedir_relative))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))

@@ -2185,6 +2217,7 @@ endif
 exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+	'-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 

@@ -2202,7 +2235,7 @@ attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 
 gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
-	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
+	-DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"'
 
 http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \
 	-DCURL_DISABLE_TYPECHECK
cache.h +1 -0
index 2e1434505..3d84aa0bf 100644
@@ -449,6 +449,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_ICASE_PATHSPECS_ENVIRONMENT "GIT_ICASE_PATHSPECS"
 #define GIT_QUARANTINE_ENVIRONMENT "GIT_QUARANTINE_PATH"
 #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
+#define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
 
 /*
  * This environment variable is expected to contain a boolean indicating
common-main.c +2 -2
index 6a689007e..6516a1f89 100644
@@ -32,12 +32,12 @@ int main(int argc, const char **argv)
 	 */
 	sanitize_stdfds();
 
+	git_resolve_executable_dir(argv[0]);
+
 	git_setup_gettext();
 
 	attr_start();
 
-	git_extract_argv0_path(argv[0]);
-
 	restore_sigpipe_to_default();
 
 	return cmd_main(argc, argv);
config.mak.uname +7 -0
index 685a80d13..58fd62b4d 100644
@@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	PROCFS_EXECUTABLE_PATH = /proc/self/exe
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease

@@ -111,6 +112,7 @@ ifeq ($(uname_S),Darwin)
 	BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
 	HAVE_BSD_SYSCTL = YesPlease
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease

@@ -206,6 +208,7 @@ ifeq ($(uname_S),FreeBSD)
 	HAVE_PATHS_H = YesPlease
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
+	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif

@@ -218,6 +221,8 @@ ifeq ($(uname_S),OpenBSD)
 	BASIC_LDFLAGS += -L/usr/local/lib
 	HAVE_PATHS_H = YesPlease
 	HAVE_BSD_SYSCTL = YesPlease
+	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
+	PROCFS_EXECUTABLE_PATH = /proc/curproc/file
 endif
 ifeq ($(uname_S),MirBSD)
 	NO_STRCASESTR = YesPlease

@@ -236,6 +241,8 @@ ifeq ($(uname_S),NetBSD)
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_PATHS_H = YesPlease
 	HAVE_BSD_SYSCTL = YesPlease
+	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
+	PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
 endif
 ifeq ($(uname_S),AIX)
 	DEFAULT_PAGER = more
exec_cmd.c +208 -31

318 lines not on this page · raw message

exec_cmd.h +2 -2
index ff0b48048..2522453cd 100644
@@ -3,8 +3,8 @@
 
 struct argv_array;
 
-extern void git_set_argv_exec_path(const char *exec_path);
-extern void git_extract_argv0_path(const char *path);
+extern void git_set_exec_path(const char *exec_path);
+extern void git_resolve_executable_dir(const char *path);
 extern const char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);
gettext.c +5 -3
index db727ea02..6b64d5c2e 100644
@@ -2,7 +2,8 @@
  * Copyright (c) 2010 Ævar Arnfjörð Bjarmason
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
+#include "exec_cmd.h"
 #include "gettext.h"
 #include "strbuf.h"
 #include "utf8.h"

@@ -157,10 +158,11 @@ static void init_gettext_charset(const char *domain)
 
 void git_setup_gettext(void)
 {
-	const char *podir = getenv("GIT_TEXTDOMAINDIR");
+	const char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);
 
 	if (!podir)
-		podir = GIT_LOCALE_PATH;
+		podir = system_path(GIT_LOCALE_PATH);
+
 	bindtextdomain("git", podir);
 	setlocale(LC_MESSAGES, "");
 	setlocale(LC_TIME, "");
git.c +1 -1
index 9e96dd409..dc4cc1419 100644
@@ -65,7 +65,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 		 */
 		if (skip_prefix(cmd, "--exec-path", &cmd)) {
 			if (*cmd == '=')
-				git_set_argv_exec_path(cmd + 1);
+				git_set_exec_path(cmd + 1);
 			else {
 				puts(git_exec_path());
 				exit(0);
Signature · 1 line
2.15.0.chromium12
#32 lore raw
In reply to Dan Jacques

On Wed, Nov 29 2017, Dan Jacques jotted:

Quoted 15 lines

This is a small update to incorporate some Windows fixes from Johannes. At this point, it passes the full test suite on Linux, Mac, and FreeBSD, as well as the Travis.ci test suites, with and without RUNTIME_PREFIX/RUNTIME_PREFIX_PERL flags.

I'm happy with the patch set, and feel that it is ready to move forward. However, while it's been looked at by several people, and I have incorporated reviewer feedback, the patch set hasn't received any formal LGTM-style responses yet. I'm not sure what standard of review is required to move forward with a patch on this project, so maybe this is totally fine, but I wanted to make sure to point this out.

I also want to note Ævar Arnfjörð Bjarmason's RFC:
https://public-inbox.org/git/[email protected]/T/

Quoted 6 lines

The proposed patch set conflicts with the Perl installation directory changes in this patch set, as avarab@ notes. The proposed Perl installation process would simplify patch 0002 in this patch set. I don't think the landing order is terribly impactful - if this lands first, the patch in the RFC would delete a few more lines, and if this lands later, patch 0002 would largely not be necessary.

In general this whole thing structurally looks good to me with the caveats noted in other review E-Mails.

I haven't done anything but skim the details of the "where's my executable" C code though, just looked at what it's doing structurally.

I think it makes sense for this to land first ahead of my patch. This is an actual feature you need, whereas I just hate our use of MakeMaker, but that can wait, unless you're keen to rebase this on my patch. Would probably make your whole diff a bit shorter.

The whole converting our absolute to relative paths in the make code is unavoidably ugly, but after having an initial knee-jerk reaction to it I don't see how it can be avoided. I was hoping most of these paths could/would just be a fixed path away from our libexec path, but alas due to having had these configurable all along that simplicity seems out of reach.

Maybe I asked this before, but I don't see any obvious reason for why RUNTIME_PREFIX_PERL is a different thing than RUNTIME_PREFIX as opposed to us just doing the right thing for the perl scripts.

author
#33 lore raw
Quoted 10 lines

In general this whole thing structurally looks good to me with the caveats noted in other review E-Mails.

I haven't done anything but skim the details of the "where's my executable" C code though, just looked at what it's doing structurally.

I think it makes sense for this to land first ahead of my patch. This is an actual feature you need, whereas I just hate our use of MakeMaker, but that can wait, unless you're keen to rebase this on my patch. Would probably make your whole diff a bit shorter.

I'm not strictly pressed for time here, so we can put this off if that's strategically appropriate. Chromiuum, right now, just manually patches upstream Git with a similar patch set, so it's working and function. This is just an effort to upstream those changes for everyone else to enjoy!

I think that landing in either order is probably okay, so if your RFC goes through pretty quickly I don't mind rebasing, but if this is otherwise good to go in v5, I wouldn't mind landing it and then removing the obsoleted parts during the Makefile update.

Quoted 6 lines

The whole converting our absolute to relative paths in the make code is unavoidably ugly, but after having an initial knee-jerk reaction to it I don't see how it can be avoided. I was hoping most of these paths could/would just be a fixed path away from our libexec path, but alas due to having had these configurable all along that simplicity seems out of reach.

Yeah I spent no small amount of time massaging that code hoping some better formulation would shake out, and this is what I ended up with. UNTIME_PREFIX_PERL is a specialty build with a stronger set of assumptions than the standard installation (RE prefix-relative paths).

Quoted 3 lines

Maybe I asked this before, but I don't see any obvious reason for why RUNTIME_PREFIX_PERL is a different thing than RUNTIME_PREFIX as opposed to us just doing the right thing for the perl scripts.

I did this to isolate Windows builds from the Perl script changes. Git-for- Windows uses (invented) RUNTIME_PREFIX, but runs its Perl scripts in a MinGW sub-environment which, internally, has the Perl libraries installed at a fixed path, so it doesn't need any special resolution logic.

I support that if Git-for-Windows wants to, a trivial future patch can merge the two, so I opted to play it safe and keep these changes isolated.

===

Quoted 3 lines

We could use $ENV{GIT_EXEC_PATH} instead of FindBin here though, I missed that the first time. But that's just a nano-optimization. I just wondered whether git wasn't already passing us this info.

Oh good idea - I'll go ahead and do this.

Quoted 9 lines

There is one remaining bug here. Git::I18N isn't doing the right thing, I installed in /tmp/git and moved to /tmp/git2, and it has:

    our $TEXTDOMAINDIR = $ENV{GIT_TEXTDOMAINDIR} || '/tmp/git/share/locale';

And GIT_TEXTDOMAINDIR is not passed by git (it's only used for the tests IIRC). Would need a similar treatment as this. Easiest to just set the path we find here in $Git::Whatever and pick it up in $Git::I18N later, it's not like anyone uses it outside of git.git.

Good catch! I'm going to see what I can do here.

Quoted 6 lines

But that does raise a more general concern for me. Isn't there some way we can run the test suite against an installed git (don't remember), then build, install, move the dir, and run the tests from the moved dir.

That would have caught this bug, and anything else that may be lurking still.

I am not aware of such an option, but I'll take a look again. This sort of thing might just require a reformulation of the test suite in general to make it run against a Git installation instead of the intermediate artifacts. A positive outcome would be the ability to remove the Perl path environment variable hacks ($ENV{...} || ...) and just use production resolution logic, increasing the test surface! But I think that's a bit more work than I have time for at the moment.

===

Quoted 8 lines

Noticed after I sent the last E-Mail, this is missing @@INSTLIBDIR@@ which per my reading of it being initially introduced should be here in addition to this relative path.

My reading of the intial patch that added it, as indicated in my patch, is that it's the dir we're going to be installing our libs to, so I didn't fiddle with it, but I think with your patches we need that dir *and* or own $perllibdir.

INSTLIBDIR is used for the standard fixed-prefix header, but not in this one. This one uses a combination of GITEXECDIR and PERLLIBDIR. PERLLIBDIR is effectively the prefix-relative part of INSTLIBDIR, so it's here in spirit!

Thanks for taking the time to review! I'll go ahead and see what I can do RE your comments.

Cheers,
-Dan