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.