Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Junio,

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

> Daniel Jacques  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

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Junio C Hamano]

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

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
> johannes.schinde...@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

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Daniel Jacques]

On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
johannes.schinde...@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

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
> johannes.schinde...@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

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Daniel Jacques]

On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
johannes.schinde...@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.

[PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

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