git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
@ 2018-03-25 20:51 Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 1/3] Makefile: generate Perl header from template file Dan Jacques
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Dan Jacques @ 2018-03-25 20:51 UTC (permalink / raw)
  To: git; +Cc: gitster, avarab, Johannes.Schindelin, Dan Jacques

This patch set expands support for the RUNTIME_PREFIX configuration flag,
currently only used on Windows builds, to include Linux, Darwin, and
FreeBSD. When Git is built with RUNTIME_PREFIX enabled, it resolves its
ancillary paths relative to the runtime location of its executable
rather than hard-coding them at compile-time, allowing a Git
installation to be deployed to a path other than the one in which it
was built/installed.

Note that RUNTIME_PREFIX is not currently used outside of Windows.
This patch set should not have an impact on default Git builds.

This is a minor update based on comments from the v6 series. If all's
well, I'm hoping this set is good to go.

Previous threads:
v1: https://public-inbox.org/git/20171116170523.28696-1-dnj@google.com/
v2: https://public-inbox.org/git/20171119173141.4896-1-dnj@google.com/
v3: https://public-inbox.org/git/20171127164055.93283-1-dnj@google.com/
v4: https://public-inbox.org/git/20171129223807.91343-1-dnj@google.com/
v5: https://public-inbox.org/git/20180108030239.92036-1-dnj@google.com/
v5: https://public-inbox.org/git/20180108030239.92036-1-dnj@google.com/
v6: https://public-inbox.org/git/20180319025046.58052-1-dnj@google.com/

Changes in v7 from v6:

- Change Perl header based on avarab@'s suggestion.
- Rearranged Makefile lines to align with avarab@'s patch in next.
- Fix typos in commit messages and comments.

=== Testing ===

The latest patch set is available for testing on my GitHub fork, including
"travis.ci" testing. The "runtime-prefix" branch includes a "config.mak"
commit that enables runtime prefix for the Travis build; the
"runtime-prefix-no-config" omits this file, testing this patch without
runtime prefix enabled:
- https://github.com/danjacques/git/tree/runtime-prefix
- https://github.com/danjacques/git/tree/runtime-prefix-no-config
- https://travis-ci.org/danjacques/git/branches

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

=== Example config.mak ===

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

=== Revision History ===

Changes in v6 from v5:

- Rebased on top of "master".
- Updated commit messages.
- Updated runtime prefix Perl header comment and code to clarify when and
  why FindBin is used.
- With Johannes' blessing on Git-for-Windows, folded "RUNTIME_PREFIX_PERL"
  functionality into "RUNTIME_PREFIX".
- Updated "run-command" test to accommodate RUNTIME_PREFIX trace messages.

Changes in v5 from v4:

- Rebase on top of "next", notably incorporating the
  "ab/simplify-perl-makefile" branch.
- Cleaner Makefile relative path enforcement.
- Update Perl header template path now that the "perl/" directory has
  fewer build-related files in it.
- Update Perl runtime prefix header to use a general system path resolution
  function.
- Implemented the injection of the locale directory into Perl's
  "Git/I18N.pm" module from the runtime prefix Perl script header.
- Updated Perl's "Git/I18N.pm" module to accept injected locale directory.
- Added more content to some comments.


Changes in v4 from v3:

- Incorporated some quoting and Makefile dependency fixes, courtesy of
  <johannes.schindelin@gmx.de>.

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 (3):
  Makefile: generate Perl header from template file
  Makefile: add Perl runtime prefix support
  exec_cmd: RUNTIME_PREFIX on some POSIX systems

 .gitignore                                       |   1 +
 Makefile                                         | 122 ++++++++++--
 cache.h                                          |   1 +
 common-main.c                                    |   4 +-
 config.mak.uname                                 |   7 +
 exec_cmd.c                                       | 236 ++++++++++++++++++++---
 exec_cmd.h                                       |   4 +-
 gettext.c                                        |   8 +-
 git.c                                            |   2 +-
 perl/Git/I18N.pm                                 |   2 +-
 perl/header_templates/fixed_prefix.template.pl   |   1 +
 perl/header_templates/runtime_prefix.template.pl |  42 ++++
 t/t0061-run-command.sh                           |   2 +-
 13 files changed, 380 insertions(+), 52 deletions(-)
 create mode 100644 perl/header_templates/fixed_prefix.template.pl
 create mode 100644 perl/header_templates/runtime_prefix.template.pl

-- 
2.15.0.chromium12


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v7 1/3] Makefile: generate Perl header from template file
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
@ 2018-03-25 20:51 ` Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 2/3] Makefile: add Perl runtime prefix support Dan Jacques
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Dan Jacques @ 2018-03-25 20:51 UTC (permalink / raw)
  To: git; +Cc: gitster, avarab, Johannes.Schindelin, Dan Jacques

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 generated
header content 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 <dnj@google.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore                                     |  1 +
 Makefile                                       | 27 +++++++++++++++-----------
 perl/header_templates/fixed_prefix.template.pl |  1 +
 3 files changed, 18 insertions(+), 11 deletions(-)
 create mode 100644 perl/header_templates/fixed_prefix.template.pl

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..89bd7bd8a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@
 /GIT-LDFLAGS
 /GIT-PREFIX
 /GIT-PERL-DEFINES
+/GIT-PERL-HEADER
 /GIT-PYTHON-VARS
 /GIT-SCRIPT-DEFINES
 /GIT-USER-AGENT
diff --git a/Makefile b/Makefile
index a1d8775ad..e479822ce 100644
--- a/Makefile
+++ b/Makefile
@@ -1975,20 +1975,15 @@ git.res: git.rc GIT-VERSION-FILE
 $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
 
 ifndef NO_PERL
-$(SCRIPT_PERL_GEN):
-
+PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
-$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
+
+$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	INSTLIBDIR='$(perllibdir_SQ)' && \
-	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' \
 	    $< >$@+ && \
@@ -2002,6 +1997,16 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
+	$(QUIET_GEN)$(RM) $@ && \
+	INSTLIBDIR='$(perllibdir_SQ)' && \
+	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
+	    -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+	    -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
+	    $< >$@+ && \
+	mv $@+ $@
 
 .PHONY: gitweb
 gitweb:
@@ -2770,7 +2775,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
diff --git a/perl/header_templates/fixed_prefix.template.pl b/perl/header_templates/fixed_prefix.template.pl
new file mode 100644
index 000000000..857b4391a
--- /dev/null
+++ b/perl/header_templates/fixed_prefix.template.pl
@@ -0,0 +1 @@
+use lib (split(/@@PATHSEP@@/, $ENV{GITPERLLIB} || '@@INSTLIBDIR@@'));
-- 
2.15.0.chromium12


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 2/3] Makefile: add Perl runtime prefix support
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 1/3] Makefile: generate Perl header from template file Dan Jacques
@ 2018-03-25 20:51 ` Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems Dan Jacques
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Dan Jacques @ 2018-03-25 20:51 UTC (permalink / raw)
  To: git; +Cc: gitster, avarab, Johannes.Schindelin, Dan Jacques

Broaden the RUNTIME_PREFIX flag to configure Git's 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. Hard-coding at build time worked on previous
RUNTIME_PREFIX configurations (i.e., Windows) because the Perl
scripts were run within a virtual filesystem whose paths were
consistent regardless of the location of the actual installation.
This will no longer be the case for non-Windows RUNTIME_PREFIX users.

When enabled, RUNTIME_PREFIX now requires Perl's system paths to be
expressed relative to a common installation directory in the Makefile,
and uses that relationship to locate support files based on the known
starting point of the script being executed, much like RUNTIME_PREFIX
does for the Git binary.

This change enables Git's Perl scripts to work when their Git installation
is relocated or moved to another system, even when they are not in a
virtual filesystem environment.

Signed-off-by: Dan Jacques <dnj@google.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile                                         | 67 +++++++++++++++++++++++-
 perl/Git/I18N.pm                                 |  2 +-
 perl/header_templates/runtime_prefix.template.pl | 42 +++++++++++++++
 3 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 perl/header_templates/runtime_prefix.template.pl

diff --git a/Makefile b/Makefile
index e479822ce..033a55505 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,13 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# 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. RUNTIME_PREFIX also causes
+# Perl scripts to use a modified entry point header allowing them to resolve
+# support files at runtime.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -471,6 +478,8 @@ ARFLAGS = rcs
 #   mandir
 #   infodir
 #   htmldir
+#   localedir
+#   perllibdir
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)
@@ -494,7 +503,10 @@ pathsep = :
 
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
+localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
+perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
 
 export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
 
@@ -1740,10 +1752,13 @@ mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
 infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 perllibdir_SQ = $(subst ','\'',$(perllibdir))
 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_relative_SQ = $(subst ','\'',$(perllibdir_relative))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
@@ -1754,6 +1769,31 @@ TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 DIFF_SQ = $(subst ','\'',$(DIFF))
 PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 
+# RUNTIME_PREFIX's resolution logic requires resource paths to be expressed
+# relative to each other and share an installation path.
+#
+# This is a dependnecy in:
+# - Git's binary RUNTIME_PREFIX logic in (see "exec_cmd.c").
+# - The runtime prefix Perl header (see
+#   "perl/header_templates/runtime_prefix.template.pl").
+ifdef RUNTIME_PREFIX
+
+ifneq ($(filter /%,$(firstword $(gitexecdir_relative))),)
+$(error RUNTIME_PREFIX requires a relative gitexecdir, not: $(gitexecdir))
+endif
+
+ifneq ($(filter /%,$(firstword $(localedir_relative))),)
+$(error RUNTIME_PREFIX requires a relative localedir, not: $(localedir))
+endif
+
+ifndef NO_PERL
+ifneq ($(filter /%,$(firstword $(perllibdir_relative))),)
+$(error RUNTIME_PREFIX requires a relative perllibdir, not: $(perllibdir))
+endif
+endif
+
+endif
+
 # We must filter out any object files from $(GITLIBS),
 # as it is typically used like:
 #
@@ -1974,10 +2014,31 @@ git.res: git.rc GIT-VERSION-FILE
 # This makes sure we depend on the NO_PERL setting itself.
 $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
 
+# Used for substitution in Perl modules. Disabled when using RUNTIME_PREFIX
+# since the locale directory is injected.
+perl_localedir_SQ = $(localedir_SQ)
+
 ifndef NO_PERL
 PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
 
+PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ)
+PERL_DEFINES += $(RUNTIME_PREFIX)
+
+# Support Perl runtime prefix. In this mode, a different header is installed
+# into Perl scripts.
+ifdef RUNTIME_PREFIX
+
+PERL_HEADER_TEMPLATE = perl/header_templates/runtime_prefix.template.pl
+
+# Don't export a fixed $(localedir) path; it will be resolved by the Perl header
+# at runtime.
+perl_localedir_SQ =
+
+endif
+
+PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
+
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1{' \
@@ -1990,6 +2051,7 @@ $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
 	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 \
@@ -2005,6 +2067,9 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 	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' \
 	    $< >$@+ && \
 	mv $@+ $@
 
@@ -2328,7 +2393,7 @@ endif
 
 perl/build/lib/%.pm: perl/%.pm
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
-	sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
+	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
 	< $< > $@
 
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index dba96fff0..bfb4fb67a 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -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));
diff --git a/perl/header_templates/runtime_prefix.template.pl b/perl/header_templates/runtime_prefix.template.pl
new file mode 100644
index 000000000..d3151d606
--- /dev/null
+++ b/perl/header_templates/runtime_prefix.template.pl
@@ -0,0 +1,42 @@
+# BEGIN RUNTIME_PREFIX generated code.
+#
+# This finds our Git::* libraries relative to the script's runtime path.
+sub __git_system_path {
+	my ($relpath) = @_;
+	my $gitexecdir_relative = '@@GITEXECDIR_REL@@';
+
+	# GIT_EXEC_PATH is supplied by `git` or the test suite.
+	my $exec_path;
+	if (exists $ENV{GIT_EXEC_PATH}) {
+		$exec_path = $ENV{GIT_EXEC_PATH};
+	} else {
+		# This can happen if this script is being directly invoked instead of run
+		# by "git".
+		require FindBin;
+		$exec_path = $FindBin::Bin;
+	}
+
+	# Trim off the relative gitexecdir path to get the system path.
+	(my $prefix = $exec_path) =~ s/\Q${gitexecdir_relative}\E$//;
+
+	require File::Spec;
+	return File::Spec->catdir($prefix, $relpath);
+}
+
+BEGIN {
+	use lib split /@@PATHSEP@@/,
+	(
+		$ENV{GITPERLLIB} ||
+		do {
+			my $perllibdir = __git_system_path('@@PERLLIBDIR_REL@@');
+			(-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_REL@@');
+}
+
+# END RUNTIME_PREFIX generated code.
-- 
2.15.0.chromium12


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v7 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 1/3] Makefile: generate Perl header from template file Dan Jacques
  2018-03-25 20:51 ` [PATCH v7 2/3] Makefile: add Perl runtime prefix support Dan Jacques
@ 2018-03-25 20:51 ` Dan Jacques
  2018-03-25 21:15 ` [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Ævar Arnfjörð Bjarmason
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Dan Jacques @ 2018-03-25 20:51 UTC (permalink / raw)
  To: git; +Cc: gitster, avarab, Johannes.Schindelin, 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 <dnj@google.com>
Thanks-to: Robbie Iannucci <iannucci@google.com>
Thanks-to: Junio C Hamano <gitster@pobox.com>
---
 Makefile               |  30 ++++++-
 cache.h                |   1 +
 common-main.c          |   4 +-
 config.mak.uname       |   7 ++
 exec_cmd.c             | 236 +++++++++++++++++++++++++++++++++++++++++++------
 exec_cmd.h             |   4 +-
 gettext.c              |   8 +-
 git.c                  |   2 +-
 t/t0061-run-command.sh |   2 +-
 9 files changed, 254 insertions(+), 40 deletions(-)

diff --git a/Makefile b/Makefile
index 033a55505..f84e816cf 100644
--- a/Makefile
+++ b/Makefile
@@ -441,6 +441,18 @@ all::
 # can be moved to arbitrary filesystem locations. RUNTIME_PREFIX also causes
 # Perl scripts to use a modified entry point header allowing them to resolve
 # support files at runtime.
+#
+# When using RUNTIME_PREFIX, define HAVE_BSD_KERN_PROC_SYSCTL if your platform
+# supports the KERN_PROC BSD sysctl function.
+#
+# When using RUNTIME_PREFIX, 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.
+#
+# When using RUNTIME_PREFIX, define HAVE_NS_GET_EXECUTABLE_PATH if your platform
+# supports calling _NSGetExecutablePath to retrieve the path of the running
+# executable.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1664,10 +1676,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
@@ -1772,7 +1797,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 # RUNTIME_PREFIX's resolution logic requires resource paths to be expressed
 # relative to each other and share an installation path.
 #
-# This is a dependnecy in:
+# This is a dependency in:
 # - Git's binary RUNTIME_PREFIX logic in (see "exec_cmd.c").
 # - The runtime prefix Perl header (see
 #   "perl/header_templates/runtime_prefix.template.pl").
@@ -2216,6 +2241,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)"'
 
@@ -2233,7 +2259,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
diff --git a/cache.h b/cache.h
index a61b2d3f0..d8c55d72b 100644
--- a/cache.h
+++ b/cache.h
@@ -428,6 +428,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"
 
 /*
  * Environment variable used in handshaking the wire protocol.
diff --git a/common-main.c b/common-main.c
index 6a689007e..6516a1f89 100644
--- a/common-main.c
+++ b/common-main.c
@@ -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);
diff --git a/config.mak.uname b/config.mak.uname
index 6a1d0de0c..e1cfe5e5e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -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
@@ -205,6 +207,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
@@ -217,6 +220,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
@@ -235,6 +240,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
diff --git a/exec_cmd.c b/exec_cmd.c
index ce192a2d6..38d52d90a 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -2,24 +2,52 @@
 #include "exec_cmd.h"
 #include "quote.h"
 #include "argv-array.h"
-#define MAX_ARGS	32
 
-static const char *argv_exec_path;
+#if defined(RUNTIME_PREFIX)
+
+#if defined(HAVE_NS_GET_EXECUTABLE_PATH)
+#include <mach-o/dyld.h>
+#endif
+
+#if defined(HAVE_BSD_KERN_PROC_SYSCTL)
+#include <sys/param.h>
+#include <sys/types.h>
+#include <sys/sysctl.h>
+#endif
+
+#endif /* RUNTIME_PREFIX */
+
+#define MAX_ARGS 32
+
+static const char *system_prefix(void);
 
 #ifdef RUNTIME_PREFIX
-static const char *argv0_path;
+
+/**
+ * When using a runtime prefix, Git dynamically resolves paths relative to its
+ * executable.
+ *
+ * The method for determining the path of the executable is highly
+ * platform-specific.
+ */
+
+/**
+ * Path to the current Git executable. Resolved on startup by
+ * 'git_resolve_executable_dir'.
+ */
+static const char *executable_dirname;
 
 static const char *system_prefix(void)
 {
 	static const char *prefix;
 
-	assert(argv0_path);
-	assert(is_absolute_path(argv0_path));
+	assert(executable_dirname);
+	assert(is_absolute_path(executable_dirname));
 
 	if (!prefix &&
-	    !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
-	    !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
-	    !(prefix = strip_path_suffix(argv0_path, "git"))) {
+	    !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&
+	    !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&
+	    !(prefix = strip_path_suffix(executable_dirname, "git"))) {
 		prefix = PREFIX;
 		trace_printf("RUNTIME_PREFIX requested, "
 				"but prefix computation failed.  "
@@ -28,27 +56,179 @@ static const char *system_prefix(void)
 	return prefix;
 }
 
-void git_extract_argv0_path(const char *argv0)
+/*
+ * Resolves the executable path from argv[0], only if it is absolute.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path_from_argv0(struct strbuf *buf, const char *argv0)
 {
 	const char *slash;
 
 	if (!argv0 || !*argv0)
-		return;
+		return -1;
 
 	slash = find_last_dir_sep(argv0);
+	if (slash) {
+		trace_printf("trace: resolved executable path from argv0: %s\n",
+			     argv0);
+		strbuf_add_absolute_path(buf, argv0);
+		return 0;
+	}
+	return -1;
+}
 
+#ifdef PROCFS_EXECUTABLE_PATH
+/*
+ * Resolves the executable path by examining a procfs symlink.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path_procfs(struct strbuf *buf)
+{
+	if (strbuf_realpath(buf, PROCFS_EXECUTABLE_PATH, 0)) {
+		trace_printf(
+			"trace: resolved executable path from procfs: %s\n",
+			buf->buf);
+		return 0;
+	}
+	return -1;
+}
+#endif /* PROCFS_EXECUTABLE_PATH */
+
+#ifdef HAVE_BSD_KERN_PROC_SYSCTL
+/*
+ * Resolves the executable path using KERN_PROC_PATHNAME BSD sysctl.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path_bsd_sysctl(struct strbuf *buf)
+{
+	int mib[4];
+	char path[MAXPATHLEN];
+	size_t cb = sizeof(path);
+
+	mib[0] = CTL_KERN;
+	mib[1] = KERN_PROC;
+	mib[2] = KERN_PROC_PATHNAME;
+	mib[3] = -1;
+	if (!sysctl(mib, 4, path, &cb, NULL, 0)) {
+		trace_printf(
+			"trace: resolved executable path from sysctl: %s\n",
+			path);
+		strbuf_addstr(buf, path);
+		return 0;
+	}
+	return -1;
+}
+#endif /* HAVE_BSD_KERN_PROC_SYSCTL */
+
+#ifdef HAVE_NS_GET_EXECUTABLE_PATH
+/*
+ * Resolves the executable path by querying Darwin application stack.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path_darwin(struct strbuf *buf)
+{
+	char path[PATH_MAX];
+	uint32_t size = sizeof(path);
+	if (!_NSGetExecutablePath(path, &size)) {
+		trace_printf(
+			"trace: resolved executable path from Darwin stack: %s\n",
+			path);
+		strbuf_addstr(buf, path);
+		return 0;
+	}
+	return -1;
+}
+#endif /* HAVE_NS_GET_EXECUTABLE_PATH */
+
+/*
+ * Resolves the absolute path of the current executable.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path(struct strbuf *buf, const char *argv0)
+{
+	/*
+	 * Identifying the executable path is operating system specific.
+	 * Selectively employ all available methods in order of preference,
+	 * preferring highly-available authoritative methods over
+	 * selectively-available or non-authoritative methods.
+	 *
+	 * All cases fall back on resolving against argv[0] if there isn't a
+	 * better functional method. However, note that argv[0] can be
+	 * used-supplied on many operating systems, and is not authoritative
+	 * in those cases.
+	 *
+	 * Each of these functions returns 0 on success, so evaluation will stop
+	 * after the first successful method.
+	 */
+	if (
+#ifdef HAVE_BSD_KERN_PROC_SYSCTL
+		git_get_exec_path_bsd_sysctl(buf) &&
+#endif /* HAVE_BSD_KERN_PROC_SYSCTL */
+
+#ifdef HAVE_NS_GET_EXECUTABLE_PATH
+		git_get_exec_path_darwin(buf) &&
+#endif /* HAVE_NS_GET_EXECUTABLE_PATH */
+
+#ifdef PROCFS_EXECUTABLE_PATH
+		git_get_exec_path_procfs(buf) &&
+#endif /* PROCFS_EXECUTABLE_PATH */
+
+		git_get_exec_path_from_argv0(buf, argv0)) {
+		return -1;
+	}
+
+	if (strbuf_normalize_path(buf)) {
+		trace_printf("trace: could not normalize path: %s\n", buf->buf);
+		return -1;
+	}
+
+	return 0;
+}
+
+void git_resolve_executable_dir(const char *argv0)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *resolved;
+	const char *slash;
+
+	if (git_get_exec_path(&buf, argv0)) {
+		trace_printf(
+			"trace: could not determine executable path from: %s\n",
+			argv0);
+		strbuf_release(&buf);
+		return;
+	}
+
+	resolved = strbuf_detach(&buf, NULL);
+	slash = find_last_dir_sep(resolved);
 	if (slash)
-		argv0_path = xstrndup(argv0, slash - argv0);
+		resolved[slash - resolved] = '\0';
+
+	executable_dirname = resolved;
+	trace_printf("trace: resolved executable dir: %s\n",
+		     executable_dirname);
 }
 
 #else
 
+/*
+ * When not using a runtime prefix, Git uses a hard-coded path.
+ */
 static const char *system_prefix(void)
 {
 	return PREFIX;
 }
 
-void git_extract_argv0_path(const char *argv0)
+/*
+ * This is called during initialization, but No work needs to be done here when
+ * runtime prefix is not being used.
+ */
+void git_resolve_executable_dir(const char *argv0)
 {
 }
 
@@ -65,32 +245,28 @@ char *system_path(const char *path)
 	return strbuf_detach(&d, NULL);
 }
 
-void git_set_argv_exec_path(const char *exec_path)
+static const char *exec_path_value;
+
+void git_set_exec_path(const char *exec_path)
 {
-	argv_exec_path = exec_path;
+	exec_path_value = exec_path;
 	/*
 	 * Propagate this setting to external programs.
 	 */
 	setenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);
 }
 
-
-/* Returns the highest-priority, location to look for git programs. */
+/* Returns the highest-priority location to look for git programs. */
 const char *git_exec_path(void)
 {
-	static char *cached_exec_path;
-
-	if (argv_exec_path)
-		return argv_exec_path;
-
-	if (!cached_exec_path) {
+	if (!exec_path_value) {
 		const char *env = getenv(EXEC_PATH_ENVIRONMENT);
 		if (env && *env)
-			cached_exec_path = xstrdup(env);
+			exec_path_value = xstrdup(env);
 		else
-			cached_exec_path = system_path(GIT_EXEC_PATH);
+			exec_path_value = system_path(GIT_EXEC_PATH);
 	}
-	return cached_exec_path;
+	return exec_path_value;
 }
 
 static void add_path(struct strbuf *out, const char *path)
@@ -103,10 +279,12 @@ static void add_path(struct strbuf *out, const char *path)
 
 void setup_path(void)
 {
+	const char *exec_path = git_exec_path();
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;
 
-	add_path(&new_path, git_exec_path());
+	git_set_exec_path(exec_path);
+	add_path(&new_path, exec_path);
 
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
@@ -125,7 +303,8 @@ const char **prepare_git_cmd(struct argv_array *out, const char **argv)
 	return out->argv;
 }
 
-int execv_git_cmd(const char **argv) {
+int execv_git_cmd(const char **argv)
+{
 	struct argv_array nargv = ARGV_ARRAY_INIT;
 
 	prepare_git_cmd(&nargv, argv);
@@ -140,8 +319,7 @@ int execv_git_cmd(const char **argv) {
 	return -1;
 }
 
-
-int execl_git_cmd(const char *cmd,...)
+int execl_git_cmd(const char *cmd, ...)
 {
 	int argc;
 	const char *argv[MAX_ARGS + 1];
diff --git a/exec_cmd.h b/exec_cmd.h
index ff0b48048..2522453cd 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -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);
diff --git a/gettext.c b/gettext.c
index db727ea02..6b64d5c2e 100644
--- a/gettext.c
+++ b/gettext.c
@@ -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, "");
diff --git a/git.c b/git.c
index ceaa58ef4..ab2042d5c 100644
--- a/git.c
+++ b/git.c
@@ -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);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 24c92b6cd..1009595d6 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -145,7 +145,7 @@ test_trace () {
 	expect="$1"
 	shift
 	GIT_TRACE=1 test-run-command "$@" run-command true 2>&1 >/dev/null | \
-		sed 's/.* run_command: //' >actual &&
+		sed -e 's/.* run_command: //' -e '/trace: .*/d' >actual &&
 	echo "$expect true" >expect &&
 	test_cmp expect actual
 }
-- 
2.15.0.chromium12


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
                   ` (2 preceding siblings ...)
  2018-03-25 20:51 ` [PATCH v7 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems Dan Jacques
@ 2018-03-25 21:15 ` Ævar Arnfjörð Bjarmason
  2018-03-26 13:03   ` Daniel Jacques
  2018-03-26  6:01 ` Junio C Hamano
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-03-25 21:15 UTC (permalink / raw)
  To: Dan Jacques; +Cc: git, gitster, Johannes.Schindelin


On Sun, Mar 25 2018, Dan Jacques wrote:

> This patch set expands support for the RUNTIME_PREFIX configuration flag,
> currently only used on Windows builds, to include Linux, Darwin, and
> FreeBSD. When Git is built with RUNTIME_PREFIX enabled, it resolves its
> ancillary paths relative to the runtime location of its executable
> rather than hard-coding them at compile-time, allowing a Git
> installation to be deployed to a path other than the one in which it
> was built/installed.
>
> Note that RUNTIME_PREFIX is not currently used outside of Windows.
> This patch set should not have an impact on default Git builds.
>
> This is a minor update based on comments from the v6 series. If all's
> well, I'm hoping this set is good to go.

This looks good to me this time around, couple of small nits (maybe
Junio can amend while queuing):

 * You add a dependnecy typo in 2/3 but fix it again in 3/3. Should be
   squashed.

 * s/\Q${gitexecdir_relative}\E$// in 2/3 can be done less verbosely as
   s/\Q$gitexecdir_relative\E$//. Discussed before in
   https://public-inbox.org/git/CAD1RUU-3Q_SYvJorU+vEY2-0CPMZ1eL-41Z6eL7Sq4USiJ0U+w@mail.gmail.com/
   seems like something you just forgot about.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
                   ` (3 preceding siblings ...)
  2018-03-25 21:15 ` [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Ævar Arnfjörð Bjarmason
@ 2018-03-26  6:01 ` Junio C Hamano
  2018-03-26 13:00   ` Daniel Jacques
  2018-03-26 21:31 ` [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design Johannes Schindelin
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2018-03-26  6:01 UTC (permalink / raw)
  To: Dan Jacques; +Cc: git, avarab, Johannes.Schindelin

Dan Jacques <dnj@google.com> writes:

> This patch set expands support for the RUNTIME_PREFIX configuration flag,
> currently only used on Windows builds, to include Linux, Darwin, and
> FreeBSD. When Git is built with RUNTIME_PREFIX enabled, it resolves its
> ancillary paths relative to the runtime location of its executable
> rather than hard-coding them at compile-time, allowing a Git
> installation to be deployed to a path other than the one in which it
> was built/installed.
>
> Note that RUNTIME_PREFIX is not currently used outside of Windows.
> This patch set should not have an impact on default Git builds.
>
> This is a minor update based on comments from the v6 series. If all's
> well, I'm hoping this set is good to go.

When testing the non-relocatable (i.e. traditional) Git, we use
GIT_EXEC_PATH and bin-wrappers/ trick to ensure that we test the
version we just have built, not a random version that happen to be
on the $PATH, without requiring the built product first to be
installed before being tested.  From the diffstat for this patchset,
I am guessing that you are using the same mechanism even when
testing the relocatable one.

I wonder if the relocatable Git would allow a simpler arrangement to
test without installing.

I am asking merely out of curiosity, not suggesting to make a trial
install somewhere in the build area and run the built Git normally
without GIT_EXEC_PATH trick.

Thanks.  

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-26  6:01 ` Junio C Hamano
@ 2018-03-26 13:00   ` Daniel Jacques
  2018-03-26 21:16     ` Johannes Schindelin
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Jacques @ 2018-03-26 13:00 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin

On Mon, Mar 26, 2018 at 2:01 AM Junio C Hamano <gitster@pobox.com> wrote:

> When testing the non-relocatable (i.e. traditional) Git, we use
> GIT_EXEC_PATH and bin-wrappers/ trick to ensure that we test the
> version we just have built, not a random version that happen to be
> on the $PATH, without requiring the built product first to be
> installed before being tested.  From the diffstat for this patchset,
> I am guessing that you are using the same mechanism even when
> testing the relocatable one.

I am - the tests will run against a runtime-prefix-enabled binary, but the
overrides supplied by the test suite will still be respected, so it should
generally behave like non-runtime-prefix in the tests.

> I wonder if the relocatable Git would allow a simpler arrangement to
> test without installing.

> I am asking merely out of curiosity, not suggesting to make a trial
> install somewhere in the build area and run the built Git normally
> without GIT_EXEC_PATH trick.

RUNTIME_PREFIX resolves paths relative to the runtime path of the Git
binary. These path expectations are constructed around installation
directories, so I'd expect that installation is a prerequisite of testing.
It's also worth noting that the runtime path resolution is
platform-dependent, so some Git-supported platforms will not be able to be
built and/or tested in this manner (at the moment). I only implemented
support for platforms I / Chromium uses, both because of my ability to test
on them and my own familiarity with them.

That said, if the test suite dropped its overrides and did a local
(testing) installation, that would be expected to work on the
RUNTIME_PREFIX binary. I assumed that one of the reasons we don't test
against an installation is b/c Git hard-codes its paths, but this would not
be a problem with RUNTIME_PREFIX enabled.

-Dan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-25 21:15 ` [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Ævar Arnfjörð Bjarmason
@ 2018-03-26 13:03   ` Daniel Jacques
  2018-03-26 14:08     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Jacques @ 2018-03-26 13:03 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Johannes Schindelin

On Sun, Mar 25, 2018 at 5:15 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com>
wrote:

> This looks good to me this time around, couple of small nits (maybe
> Junio can amend while queuing):

>   * You add a dependnecy typo in 2/3 but fix it again in 3/3. Should be
>     squashed.

d'oh, I'll fix that in my local copy so that if I do end up needing to
upload a new version, it's available.

>   * s/\Q${gitexecdir_relative}\E$// in 2/3 can be done less verbosely as
>     s/\Q$gitexecdir_relative\E$//. Discussed before in

https://public-inbox.org/git/CAD1RUU-3Q_SYvJorU+vEY2-0CPMZ1eL-41Z6eL7Sq4USiJ0U+w@mail.gmail.com/
>     seems like something you just forgot about.

Oh sorry, I must have missed that. I have a personal preference for adding
brackets for clarity; it leaked into this patch set. I did implement most
of the suggestion, which was to use the escaped Q/E instead of equals.

Stylistically I still prefer the braces, but I'll defer to you and remove
them in my pending patch set in case I'm asked to submit another version.

Cheers!
-Dan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-26 13:03   ` Daniel Jacques
@ 2018-03-26 14:08     ` Ævar Arnfjörð Bjarmason
  2018-03-26 14:55       ` Daniel Jacques
  0 siblings, 1 reply; 20+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-03-26 14:08 UTC (permalink / raw)
  To: Daniel Jacques; +Cc: git, Junio C Hamano, Johannes Schindelin


On Mon, Mar 26 2018, Daniel Jacques wrote:

> On Sun, Mar 25, 2018 at 5:15 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> wrote:
>
>> This looks good to me this time around, couple of small nits (maybe
>> Junio can amend while queuing):
>
>>   * You add a dependnecy typo in 2/3 but fix it again in 3/3. Should be
>>     squashed.
>
> d'oh, I'll fix that in my local copy so that if I do end up needing to
> upload a new version, it's available.

\o/

>>   * s/\Q${gitexecdir_relative}\E$// in 2/3 can be done less verbosely as
>>     s/\Q$gitexecdir_relative\E$//. Discussed before in
>
> https://public-inbox.org/git/CAD1RUU-3Q_SYvJorU+vEY2-0CPMZ1eL-41Z6eL7Sq4USiJ0U+w@mail.gmail.com/
>>     seems like something you just forgot about.
>
> Oh sorry, I must have missed that. I have a personal preference for adding
> brackets for clarity; it leaked into this patch set. I did implement most
> of the suggestion, which was to use the escaped Q/E instead of equals.
>
> Stylistically I still prefer the braces, but I'll defer to you and remove
> them in my pending patch set in case I'm asked to submit another version.

If you prefer it that way just keep your version. It's your code and
it's just a trivial style difference.

I just mentioned it because in the previous discussion you said "I agree
it's cleaner" so I inferred that you'd just forgotten about it but meant
to change it. It's also fine if later you just thought "you know what,
I'm doing it my way" :)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-26 14:08     ` Ævar Arnfjörð Bjarmason
@ 2018-03-26 14:55       ` Daniel Jacques
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Jacques @ 2018-03-26 14:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Johannes Schindelin

On Mon, Mar 26, 2018 at 10:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com>
wrote:

> > Oh sorry, I must have missed that. I have a personal preference for
adding
> > brackets for clarity; it leaked into this patch set. I did implement
most
> > of the suggestion, which was to use the escaped Q/E instead of equals.
> >
> > Stylistically I still prefer the braces, but I'll defer to you and
remove
> > them in my pending patch set in case I'm asked to submit another
version.

> If you prefer it that way just keep your version. It's your code and
> it's just a trivial style difference.

> I just mentioned it because in the previous discussion you said "I agree
> it's cleaner" so I inferred that you'd just forgotten about it but meant
> to change it. It's also fine if later you just thought "you know what,
> I'm doing it my way" :)

Honestly there's enough of a delay here that I don't remember, but if I had
to guess I probably just forgot to make the change :)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git
  2018-03-26 13:00   ` Daniel Jacques
@ 2018-03-26 21:16     ` Johannes Schindelin
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-26 21:16 UTC (permalink / raw)
  To: Daniel Jacques
  Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason

Hi,

On Mon, 26 Mar 2018, Daniel Jacques wrote:

> On Mon, Mar 26, 2018 at 2:01 AM Junio C Hamano <gitster@pobox.com> wrote:
> 
> > I wonder if the relocatable Git would allow a simpler arrangement to
> > test without installing.
> 
> > I am asking merely out of curiosity, not suggesting to make a trial
> > install somewhere in the build area and run the built Git normally
> > without GIT_EXEC_PATH trick.
> 
> RUNTIME_PREFIX resolves paths relative to the runtime path of the Git
> binary. These path expectations are constructed around installation
> directories, so I'd expect that installation is a prerequisite of testing.

Indeed. This is the relevant part of the code:

        if (!prefix &&
            !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
            !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
            !(prefix = strip_path_suffix(argv0_path, "git"))) {
                prefix = FALLBACK_RUNTIME_PREFIX;
                trace_printf("RUNTIME_PREFIX requested, "
                                "but prefix computation failed.  "
                                "Using static fallback '%s'.\n", prefix);
        }

Note how the argv0_path (which is the absolute path of the directory
*containing* the `git` executable) is tested for several suffixes, i.e.
trailing directories, namely

	libexec/git-core
	bin
	git

That means that you will have to have your `git` executable built in a
worktree whose absolute path ends in one of these.

While writing this reply, I was wondering why "git" is included in this
list. You know, I can see libexec/git-core and bin because that is where
the `git` executable is installed to (or hard-linked to). But "git"?

Turns out that I am the responsible person for that (024aa7d8d51
(system_path(): simplify using strip_path_suffix(), and add suffix "git",
2009-02-19)), having assumed back then that everybody who uses the
RUNTIME_PREFIX feature and works on Git does so in /git/. Which is of
course no longer true in general. For example, I myself got bitten by this
when developing some patches on top of Dan's patch series in a *linked
worktree*  of the name "runtime-prefix". Oh well.

But the short answer is: no, you cannot rely on the RUNTIME_PREFIX feature
for running Git's own test suite. The GIT_EXEC_PATH method to force Git's
test suite to use the compiled executables is still required.

Even if it is fragile: if git-FOO exists in <prefix>/bin/ and some test
relies on the FOO subcommand but we removed it from the source code to
test whether it is needed, the test suite would pass just fine because it
finds git-FOO in the PATH.

*sigh* seems that I cannot write short answers.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
                   ` (4 preceding siblings ...)
  2018-03-26  6:01 ` Junio C Hamano
@ 2018-03-26 21:31 ` Johannes Schindelin
  2018-03-27 14:37   ` Daniel Jacques
  2018-03-26 21:31 ` [PATCH 1/2] exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows Johannes Schindelin
  2018-03-26 21:31 ` [PATCH 2/2] mingw/msvc: use the new-style RUNTIME_PREFIX helper Johannes Schindelin
  7 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-26 21:31 UTC (permalink / raw)
  To: Dan Jacques; +Cc: git, gitster

Even if the RUNTIME_PREFIX feature originates from Git for Windows, the
current patch series is different enough in its design that it leaves the
Windows-specific RUNTIME_PREFIX handling in place: On Windows, we still
have to override argv[0] with the absolute path of the current `git`
executable.

Let's just port the Windows-specific code over to the new design and get
rid of that argv[0] overwriting.

This also partially addresses a very obscure problem reported on the Git
for Windows bug tracker, where misspelling a builtin command using a
creative mIxEd-CaSe version could lead to an infinite ping-pong between
git.exe and Git for Windows' "Git wrapper" (that we use in place of
copies when on a file system without hard-links, most notably FAT).

Dan, I would be delighted if you could adopt these patches into your patch
series.

Johannes Schindelin (2):
  exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows
  mingw/msvc: use the new-style RUNTIME_PREFIX helper

 Makefile         |  8 ++++++++
 compat/mingw.c   |  5 ++---
 config.mak.uname |  2 ++
 exec_cmd.c       | 22 ++++++++++++++++++++++
 4 files changed, 34 insertions(+), 3 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
                   ` (5 preceding siblings ...)
  2018-03-26 21:31 ` [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design Johannes Schindelin
@ 2018-03-26 21:31 ` Johannes Schindelin
  2018-03-26 21:31 ` [PATCH 2/2] mingw/msvc: use the new-style RUNTIME_PREFIX helper Johannes Schindelin
  7 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-26 21:31 UTC (permalink / raw)
  To: Dan Jacques; +Cc: git, gitster

The RUNTIME_PREFIX feature comes from Git for Windows, but it was
enhanced to allow support for other platforms. While changing the
original idea, the concept was also improved by not forcing argv[0] to
be adjusted.

Let's allow the same for Windows by implementing a helper just as for
the other platforms.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile   |  8 ++++++++
 exec_cmd.c | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/Makefile b/Makefile
index f84e816..c944168 100644
--- a/Makefile
+++ b/Makefile
@@ -453,6 +453,10 @@ all::
 # When using RUNTIME_PREFIX, define HAVE_NS_GET_EXECUTABLE_PATH if your platform
 # supports calling _NSGetExecutablePath to retrieve the path of the running
 # executable.
+#
+# When using RUNTIME_PREFIX, define HAVE_WPGMPTR if your platform offers
+# the global variable _wpgmptr containing the absolute path of the current
+# executable (this is the case on Windows).
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1693,6 +1697,10 @@ ifdef HAVE_NS_GET_EXECUTABLE_PATH
 	BASIC_CFLAGS += -DHAVE_NS_GET_EXECUTABLE_PATH
 endif
 
+ifdef HAVE_WPGMPTR
+	BASIC_CFLAGS += -DHAVE_WPGMPTR
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
diff --git a/exec_cmd.c b/exec_cmd.c
index 38d52d9..6e114f8 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -144,6 +144,24 @@ static int git_get_exec_path_darwin(struct strbuf *buf)
 }
 #endif /* HAVE_NS_GET_EXECUTABLE_PATH */
 
+#ifdef HAVE_WPGMPTR
+/*
+ * Resolves the executable path by using the global variable _wpgmptr.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int git_get_exec_path_wpgmptr(struct strbuf *buf)
+{
+	int len = wcslen(_wpgmptr) * 3 + 1;
+	strbuf_grow(buf, len);
+	len = xwcstoutf(buf->buf, _wpgmptr, len);
+	if (len < 0)
+		return -1;
+	buf->len += len;
+	return 0;
+}
+#endif /* HAVE_WPGMPTR */
+
 /*
  * Resolves the absolute path of the current executable.
  *
@@ -178,6 +196,10 @@ static int git_get_exec_path(struct strbuf *buf, const char *argv0)
 		git_get_exec_path_procfs(buf) &&
 #endif /* PROCFS_EXECUTABLE_PATH */
 
+#ifdef HAVE_WPGMPTR
+		git_get_exec_path_wpgmptr(buf) &&
+#endif /* HAVE_WPGMPTR */
+
 		git_get_exec_path_from_argv0(buf, argv0)) {
 		return -1;
 	}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/2] mingw/msvc: use the new-style RUNTIME_PREFIX helper
  2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
                   ` (6 preceding siblings ...)
  2018-03-26 21:31 ` [PATCH 1/2] exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows Johannes Schindelin
@ 2018-03-26 21:31 ` Johannes Schindelin
  7 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-26 21:31 UTC (permalink / raw)
  To: Dan Jacques; +Cc: git, gitster

This change also allows us to stop overriding argv[0] with the absolute
path of the executable, allowing us to preserve e.g. the case of the
executable's file name.

This fixes https://github.com/git-for-windows/git/issues/1496 partially.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c   | 5 ++---
 config.mak.uname | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a67872b..6ded1c8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2221,7 +2221,7 @@ void mingw_startup(void)
 		die_startup();
 
 	/* determine size of argv and environ conversion buffer */
-	maxlen = wcslen(_wpgmptr);
+	maxlen = wcslen(wargv[0]);
 	for (i = 1; i < argc; i++)
 		maxlen = max(maxlen, wcslen(wargv[i]));
 	for (i = 0; wenv[i]; i++)
@@ -2241,8 +2241,7 @@ void mingw_startup(void)
 	buffer = malloc_startup(maxlen);
 
 	/* convert command line arguments and environment to UTF-8 */
-	__argv[0] = wcstoutfdup_startup(buffer, _wpgmptr, maxlen);
-	for (i = 1; i < argc; i++)
+	for (i = 0; i < argc; i++)
 		__argv[i] = wcstoutfdup_startup(buffer, wargv[i], maxlen);
 	for (i = 0; wenv[i]; i++)
 		environ[i] = wcstoutfdup_startup(buffer, wenv[i], maxlen);
diff --git a/config.mak.uname b/config.mak.uname
index e1cfe5e..a6e734c 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -357,6 +357,7 @@ ifeq ($(uname_S),Windows)
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 	NO_SVN_TESTS = YesPlease
 	RUNTIME_PREFIX = YesPlease
+	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -506,6 +507,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
+	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-26 21:31 ` [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design Johannes Schindelin
@ 2018-03-27 14:37   ` Daniel Jacques
  2018-03-27 15:54     ` Johannes Schindelin
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Jacques @ 2018-03-27 14:37 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
johannes.schindelin@gmx.de> wrote:

> Even if the RUNTIME_PREFIX feature originates from Git for Windows, the
> current patch series is different enough in its design that it leaves the
> Windows-specific RUNTIME_PREFIX handling in place: On Windows, we still
> have to override argv[0] with the absolute path of the current `git`
> executable.

> Let's just port the Windows-specific code over to the new design and get
> rid of that argv[0] overwriting.

> This also partially addresses a very obscure problem reported on the Git
> for Windows bug tracker, where misspelling a builtin command using a
> creative mIxEd-CaSe version could lead to an infinite ping-pong between
> git.exe and Git for Windows' "Git wrapper" (that we use in place of
> copies when on a file system without hard-links, most notably FAT).

> Dan, I would be delighted if you could adopt these patches into your patch
> series.

Great, I'm glad this patch set could be useful to you! I'm happy to apply
this to the patch series. They applied cleanly, so I'll push a new version
after Travis validates the candidate.

I don't have a Windows testing facility available, so I'm hoping that you
verified that this works locally. I suppose that's what the unstable branch
series is for.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-27 14:37   ` Daniel Jacques
@ 2018-03-27 15:54     ` Johannes Schindelin
  2018-03-27 16:05       ` Daniel Jacques
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-27 15:54 UTC (permalink / raw)
  To: Daniel Jacques; +Cc: git, Junio C Hamano

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
> johannes.schindelin@gmx.de> wrote:
> 
> > Even if the RUNTIME_PREFIX feature originates from Git for Windows, the
> > current patch series is different enough in its design that it leaves the
> > Windows-specific RUNTIME_PREFIX handling in place: On Windows, we still
> > have to override argv[0] with the absolute path of the current `git`
> > executable.
> 
> > Let's just port the Windows-specific code over to the new design and get
> > rid of that argv[0] overwriting.
> 
> > This also partially addresses a very obscure problem reported on the Git
> > for Windows bug tracker, where misspelling a builtin command using a
> > creative mIxEd-CaSe version could lead to an infinite ping-pong between
> > git.exe and Git for Windows' "Git wrapper" (that we use in place of
> > copies when on a file system without hard-links, most notably FAT).
> 
> > Dan, I would be delighted if you could adopt these patches into your patch
> > series.
> 
> Great, I'm glad this patch set could be useful to you! I'm happy to apply
> this to the patch series. They applied cleanly, so I'll push a new version
> after Travis validates the candidate.
> 
> I don't have a Windows testing facility available, so I'm hoping that you
> verified that this works locally. I suppose that's what the unstable branch
> series is for.

Yes, I performed manual testing.

I guess we should add a test where we copy the `git` executable into a
subdirectory with the name "git" and call `git/git --exec-path` and verify
that its output matches our expectation?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-27 15:54     ` Johannes Schindelin
@ 2018-03-27 16:05       ` Daniel Jacques
  2018-03-27 16:28         ` Johannes Schindelin
  2018-03-28 17:12         ` Junio C Hamano
  0 siblings, 2 replies; 20+ messages in thread
From: Daniel Jacques @ 2018-03-27 16:05 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
Johannes.Schindelin@gmx.de> wrote:

> Yes, I performed manual testing.

Alright! Just manually tested your "git" scenario myself on the Linux build
and all seems to be in order.

> I guess we should add a test where we copy the `git` executable into a
> subdirectory with the name "git" and call `git/git --exec-path` and verify
> that its output matches our expectation?

I'm actually a little fuzzy on the testing model here. As things are, this
test will only work if Git is relocatable; however, the test suite doesn't
seem to be equipped to build multiple versions of Git for different tests.
 From this I conclude that the right approach would be to make a test that
runs conditional on RUNTIME_PREFIX being set, but I'm not familiar enough
with the testing framework to be confident that this is correct, or really
how to go about writing such a test.

A simple grep suggests that the current test suite doesn't seem to have any
RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
been doing it with a "config.mak" file that explicitly enables
RUNTIME_PREFIX to get the runtime prefix code tested against the standard
Git testing suites.

 From a Git maintainer's perspective, would such a test be a prerequisite
for landing this patch series, or is this a good candidate for follow-up
work to improve our testing coverage?

-Dan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-27 16:05       ` Daniel Jacques
@ 2018-03-27 16:28         ` Johannes Schindelin
  2018-03-28 17:12         ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-27 16:28 UTC (permalink / raw)
  To: Daniel Jacques; +Cc: git, Junio C Hamano

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
> Johannes.Schindelin@gmx.de> wrote:
> 
> > I guess we should add a test where we copy the `git` executable into a
> > subdirectory with the name "git" and call `git/git --exec-path` and
> > verify that its output matches our expectation?
> 
> I'm actually a little fuzzy on the testing model here.

Alright, I'll bite.

You are correct that the test must be contingent on the RUNTIME_PREFIX
prerequisite. This could be tested thusly:

	test_lazy_prereq RUNTIME_PREFIX '
		# test whether we built with RUNTIME_PREFIX support
		grep " -DRUNTIME_PREFIX" "$GIT_BUILD_DIR/GIT-CFLAGS"
	'

The subsequent test would run like this:

	test_expect_success RUNTIME_PREFIX '
		mkdir git &&
		cp "$GIT_BUILD_DIR/git$X" git/ &&
		path="$(git/git$X --exec-path)" &&
		case "$(echo "$path" | tr '\\' /)" in
		"$(pwd)/libexec/git-core") ;; # okay
		*)
			echo "Unexpected exec path: $path" >&2
			return 1
			;;
		esac
	'

I say "like this" because it is a little bit tricky to get right, in
particular when supporting Windows ;-)

For example, when building with Visual C, the dependencies' .dll files
need to be copied into the same directory as the .exe files because there
is no good central place to put them (don't get me started on the problems
incurred by some software copying some random OpenSSL version's
ssleay32.dll into C:\Windows\system32, unless you buy me beer all night
and want to be entertained). And that obviously would fail with this
approach.

> As things are, this test will only work if Git is relocatable; however,
> the test suite doesn't seem to be equipped to build multiple versions of
> Git for different tests.  From this I conclude that the right approach
> would be to make a test that runs conditional on RUNTIME_PREFIX being
> set, but I'm not familiar enough with the testing framework to be
> confident that this is correct, or really how to go about writing such a
> test.
> 
> A simple grep suggests that the current test suite doesn't seem to have any
> RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> been doing it with a "config.mak" file that explicitly enables
> RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> Git testing suites.

Indeed, this would be the first test.

>  From a Git maintainer's perspective, would such a test be a
>  prerequisite for landing this patch series, or is this a good candidate
>  for follow-up work to improve our testing coverage?

I cannot speak for Junio, but from my understanding he would probably be
fine without such a test. Or a separate patch at a later stage that
introduces that.

Or something completely different such as a helper in t/helper/ that
always succeeds if RUNTIME_PREFIX is not defined, otherwise passes argv[1]
as parameter to git_resolve_executable_dir() and outputs that. Would be a
lot more robust than what I described above. But I would want for Duy's
test-tool patch series to land first because I would hate to introduce
*yet* another stand-alone .exe in t/helper/.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-27 16:05       ` Daniel Jacques
  2018-03-27 16:28         ` Johannes Schindelin
@ 2018-03-28 17:12         ` Junio C Hamano
  2018-03-29 14:54           ` Johannes Schindelin
  1 sibling, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2018-03-28 17:12 UTC (permalink / raw)
  To: Daniel Jacques; +Cc: Johannes Schindelin, git

Daniel Jacques <dnj@google.com> writes:

> A simple grep suggests that the current test suite doesn't seem to have any
> RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> been doing it with a "config.mak" file that explicitly enables
> RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> Git testing suites.
>
> From a Git maintainer's perspective, would such a test be a prerequisite
> for landing this patch series, or is this a good candidate for follow-up
> work to improve our testing coverage?

It would be a nice-to-have follow-up, I would say, but as you two
seem to be working well together and it shouldn't be too involved to
have the minimum test that makes sure the version of "git" being
tested thinks things should be where we think they should be, with
something like...

	test_expect_success RUNTIME_PREFIX 'runtime-prefix basics' '
		(
			# maybe others
			safe_unset GIT_EXEC_PATH &&
			git --exec-path >actual
		) &&
		# compute the expected value -- we know the first
		# element of $PATH is where we find "git", so things
		# should be computable relative to that, perhaps?
		echo >expect "${PATH%%:*}/..." &&
		# then compare
		test_cmp expect actual		
	'

so I am hoping such a minimum test to be in the series when it
graduate to 'master' and become a part of a release.  

On the other hand, "make a whole test install and try running it"
may actually be easier but that probably can be done using existing
GIT_TEST_INSTALLED framework?  In short, you would probably do

 - make RUNTIME_PREFIX=YesPlease
 - make RUNTIME_PREFIX=YesPlease DESTDIR=...some..where... install
 - GIT_TEST_INSTALLED=...some..where.../bin make test

or something like that.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design
  2018-03-28 17:12         ` Junio C Hamano
@ 2018-03-29 14:54           ` Johannes Schindelin
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-03-29 14:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Daniel Jacques, git

Hi Junio,

On Wed, 28 Mar 2018, Junio C Hamano wrote:

> Daniel Jacques <dnj@google.com> writes:
> 
> > A simple grep suggests that the current test suite doesn't seem to have any
> > RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> > been doing it with a "config.mak" file that explicitly enables
> > RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> > Git testing suites.
> >
> > From a Git maintainer's perspective, would such a test be a prerequisite
> > for landing this patch series, or is this a good candidate for follow-up
> > work to improve our testing coverage?
> 
> It would be a nice-to-have follow-up, I would say, but as you two
> seem to be working well together and it shouldn't be too involved to
> have the minimum test that makes sure the version of "git" being
> tested thinks things should be where we think they should be, with
> something like...
> 
> 	test_expect_success RUNTIME_PREFIX 'runtime-prefix basics' '
> 		(
> 			# maybe others
> 			safe_unset GIT_EXEC_PATH &&
> 			git --exec-path >actual

That will only work when the directory into which git (or git.exe) was
compiled is called "bin" or "git" (or "git-core" in a "libexec"
directory), because this is the sanity check we have to determine that Git
is installed into a sensible location where we *can* assume that
libexec/git-core/ is the corresponding location of the support
executables/scripts.

I initially thought that we could somehow do this:

-- snip --
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 2b3c5092a19..3040f0dae49 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "string-list.h"
+#include "exec_cmd.h"
 
 /*
  * A "string_list_each_func_t" function that normalizes an entry from
@@ -270,6 +271,25 @@ int cmd_main(int argc, const char **argv)
 	if (argc == 2 && !strcmp(argv[1], "dirname"))
 		return test_function(dirname_data, posix_dirname,
argv[1]);
 
+	if (argc == 3 && !strcmp(argv[1], "runtime-prefix")) {
+#ifndef RUNTIME_PREFIX
+		warning("RUNTIME_PREFIX support not compiled in;
skipping");
+		return 0;
+#else
+		char *path;
+
+		git_resolve_executable_dir(argv[2]);
+		path = system_path("");
+
+		if (!starts_with(argv[2], path))
+			return error("unexpected prefix: '%s'", path);
+
+		puts(path);
+
+		return 0;
+#endif
+	}
+
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
```

but this simply won't work, as the main idea of
`git_resolve_executable_dir()` is to use the executable path whenever
possible, instead of the passed-in parameter.

And since we usually work via the bin-wrappers, we cannot even add a
sanity check that Git was cloned into a directory called "git"...

So... I think we have to leave this out of the patch series, unless
somebody comes up with an idea neither Dan nor I has thought about to test
this reliably *without* copying the Git executable (which would, as I
mentioned, break testing when .dll files need to be present in the same
directory as git.exe).

Ciao,
Dscho



^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-03-29 14:54 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-25 20:51 [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
2018-03-25 20:51 ` [PATCH v7 1/3] Makefile: generate Perl header from template file Dan Jacques
2018-03-25 20:51 ` [PATCH v7 2/3] Makefile: add Perl runtime prefix support Dan Jacques
2018-03-25 20:51 ` [PATCH v7 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems Dan Jacques
2018-03-25 21:15 ` [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git Ævar Arnfjörð Bjarmason
2018-03-26 13:03   ` Daniel Jacques
2018-03-26 14:08     ` Ævar Arnfjörð Bjarmason
2018-03-26 14:55       ` Daniel Jacques
2018-03-26  6:01 ` Junio C Hamano
2018-03-26 13:00   ` Daniel Jacques
2018-03-26 21:16     ` Johannes Schindelin
2018-03-26 21:31 ` [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design Johannes Schindelin
2018-03-27 14:37   ` Daniel Jacques
2018-03-27 15:54     ` Johannes Schindelin
2018-03-27 16:05       ` Daniel Jacques
2018-03-27 16:28         ` Johannes Schindelin
2018-03-28 17:12         ` Junio C Hamano
2018-03-29 14:54           ` Johannes Schindelin
2018-03-26 21:31 ` [PATCH 1/2] exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows Johannes Schindelin
2018-03-26 21:31 ` [PATCH 2/2] mingw/msvc: use the new-style RUNTIME_PREFIX helper Johannes Schindelin

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).