Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Johannes Schindelin]

Hi,

On Mon, 26 Mar 2018, Daniel Jacques wrote:

> On Mon, Mar 26, 2018 at 2:01 AM Junio C Hamano  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 /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

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Daniel Jacques]

On Mon, Mar 26, 2018 at 10:08 AM Ævar Arnfjörð Bjarmason 
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 :)

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 26 2018, Daniel Jacques wrote:

> On Sun, Mar 25, 2018 at 5:15 PM Ævar Arnfjörð Bjarmason 
> 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-41z6el7sq4usij0...@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" :)

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Daniel Jacques]

On Sun, Mar 25, 2018 at 5:15 PM Ævar Arnfjörð Bjarmason 
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-41z6el7sq4usij0...@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

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Daniel Jacques]

On Mon, Mar 26, 2018 at 2:01 AM Junio C Hamano  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

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Junio C Hamano]

Dan Jacques  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.  

Re: [PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[Ævar Arnfjörð Bjarmason]

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-41z6el7sq4usij0...@mail.gmail.com/
   seems like something you just forgot about.

[PATCH v7 0/3] RUNTIME_PREFIX relocatable Git

[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-...@google.com/
v2: https://public-inbox.org/git/20171119173141.4896-1-...@google.com/
v3: https://public-inbox.org/git/20171127164055.93283-1-...@google.com/
v4: https://public-inbox.org/git/20171129223807.91343-1-...@google.com/
v5: https://public-inbox.org/git/20180108030239.92036-1-...@google.com/
v5: https://public-inbox.org/git/20180108030239.92036-1-...@google.com/
v6: https://public-inbox.org/git/20180319025046.58052-1-...@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
  .

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: