Re: Git 2.18: RUNTIME_PREFIX... is it working?

[Daniel Jacques]

Perry Hutchison wrote:

> If we need /proc, wouldn't we _already_ be unhappy inside a chroot
> that didn't mount /proc, even _with_ fallback to static paths?
> Last I knew, the whole point of chroots/containers/jails/etc. was to
> prevent access, from a process running inside the container, to any
> part of the FS that's outside of the container.

Yep. The code also allows for use of "argv[0]", but that has its own set of
problems. These were previously covered in the discussions leading up to
the patch landing, but to summarize, "argv[0]" can be completely manipulated
by the calling process, whimsically or in response to constraints such as
links, and is wholly unreliable for self-location.

Other kernels have their own behaviors with respect to path self-location,
and none seem to be straightforward. This link seems to have a good
rundown of the details and differences:

https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe

All things considered, I think executable path self-location is markedly more
fragile than using static paths, both with increased dependencies and added
inconsistent behavior and limitations, and should not be the default
on any platform.

Both Johannes' original RUNTIME_PREFIX implementation for Windows and the
Linux/etc. expansions that I did were written to serve constrained special case
deployments. In that capacity, they can be really useful, as the fragility is
managed by their respective environments.

My particular use case served the purpose of building Git once and deploying
it via archive on other systems. This capability requires the additional work of
building portable versions of Git's dependencies and their associated
resources, and statically linking everything together.

This is a lot more portability than the conventional user requires, and also
necessitates a significantly more complex build process. However, making Git
itself portable via RUNTIME_PREFIX without similar work on its dependencies
limits the usefulness of that portability, since it's still bound to
the system's
libraries and their resources.

Cheers,
-Dan

Re: Git 2.18: RUNTIME_PREFIX... is it working?

[Daniel Jacques]

Apologies for the delayed response - I've been out of town - and It
looks like Paul is already on the right track.

Johannes: I believe the GIT_EXEC_PATH snipped that you listed is not
incorrect. It's defined to "gitexecdir_SQ", and RUNTIME_PREFIX expects
(and enforces, as you snipped) that this is a relative path in
Makefile.

On non-RUNTIME_PREFIX builds, it should still be the absolute path, as
this is how Git self-locates, so using "gitexecdir_SQ" there makes
sense to me.

So:
RUNTIME_PREFIX=No, gitexecdir_SQ is absolute, GIT_EXEC_PATH is
absolute, used to find Git:
https://github.com/git/git/blob/ccdcbd54c4475c2238b310f7113ab3075b5abc9c/exec-cmd.c#L281
RUNTIME_PREFIX=YesPlease, gitexecdir_SQ is relative, GIT_EXEC_PATH is
relative and used to identify the search root of the Git installation:
https://github.com/git/git/blob/ccdcbd54c4475c2238b310f7113ab3075b5abc9c/exec-cmd.c#L40

The dual-use is confusing, and it took me a few to walk back through
how it is employed in each scenario. For clarity's sake, it may be
worth defining two variables and making one explicitly relative, but I
think it is functional as-is.

Paul: I used "config.mak" to configure RUNTIME_PREFIX when I used it
to the same effect:
https://chromium.googlesource.com/infra/infra/+/ca729b99c1f82665b634ef2ff69d93c97dfcda99/recipes/recipe_modules/third_party_packages/git.py#78

I forewent autoconf because I was concerned that the option was too
obscure and the configuration too nuanced to be worth adding via flag,
as RUNTIME_PREFIX requires some degree of path alignment and is fairly
special-case. If you prefer autoconf, though, it sounds like a good
thing to add, and I'm happy that you are finding the feature useful!

Cheers,
-Dan

On Fri, Jul 6, 2018 at 5:00 AM Johannes Schindelin
 wrote:
>
> Hi Paul,
>
> On Thu, 5 Jul 2018, Paul Smith wrote:
>
> > On Wed, 2018-07-04 at 13:22 +0200, Johannes Schindelin wrote:
> > > > Basically what happens is that I run configure with
> > > > --prefix=/my/install/path --with-gitconfig=etc/gitconfig
> > > > --with-gitattributes=etc/gitattributes.
> > > >
> > > > Then I run make with RUNTIME_PREFIX=YesPlease.
> > >
> > > Ah. In Git for Windows, we do not use configure. I *think* this
> > > points to an incompatibility of the RUNTIME_PREFIX feature with our
> > > autoconf support, and this is a grand opportunity for you to step in
> > > and help.
> > >
> > > Essentially, what you will want to do is to implement a new configure
> > > option --with-runtime-prefix that then prevents the autoconf script
> > > from munging the relative paths in the way it does.
> >
> > FYI I was able to get this to work by overriding variables on the make
> > command line, like this:
> >
> >   make ... RUNTIME_PREFIX=YesPlease \
> >   gitexecdir=libexec/git-core \
> >   template_dir=share/git-core/templates \
> >   sysconfdir=etc
> >
> > I agree a new autoconf option would be much simpler to use.  I'll think
> > about it as I happen to have some some experience in these areas ;) ...
>
> I look forward to reviewing this...
>
> > but time is limited of course :).
>
> Yep. Same here ;-)
>
> Ciao,
> Johannes

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

[Daniel Jacques]

Good catch, thanks for doing this!
-Dan
On Mon, Apr 23, 2018 at 10:18 PM Jonathan Nieder  wrote:

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

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

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

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

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

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

> Improved-by: Junio C Hamano 
> Signed-off-by: Jonathan Nieder 
> ---
> Hi,

> Junio C Hamano wrote:
> > Jonathan Nieder  writes:

> >> +++ b/Makefile
> >> @@ -2108,7 +2108,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)
GIT-PERL-DEFINES Makefile
> >>  INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> >>
  INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
> >>  sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> >> --e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> >> +-e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
> >
> > Good find.  FWIW, I'd find it a lot easier to read if the whole
> > thing were enclosed inside a single pair of dq.

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

>   Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/Makefile b/Makefile
> index 2327ccb906..5e25441861 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2116,7 +2116,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE)
GIT-PERL-DEFINES Makefile
>  INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \

INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>  sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> -   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> +   -e "s=@@INSTLIBDIR@@=$$INSTLIBDIR=g" \
>  -e 's=@@PERLLIBDIR@@='$(perllibdir_SQ)'=g' \
>  -e 's=@@PERLLIBDIR_REL@@=$(perllibdir_relative_SQ)=g' \
>  -e 's=@@GITEXECDIR_REL@@=$(gitexecdir_relative_SQ)=g' \
> --
> 2.17.0.441.gb46fe60e1d

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

[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.

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

[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 v6 2/3] Makefile: add Perl runtime prefix support

[Daniel Jacques]

On Mon, Mar 19, 2018 at 5:32 PM Martin Ågren  wrote:

> This commit message mentions RUNTIME_PREFIX_PERL twice, but there is no
> use of RUNTIME_PREFIX_PERL in the actual diffs (patches 1-3/3). Should
> it be s/_PERL//? Your cover letter hints as much under "Changes in v6
> from v5". And "Add a new Makefile flag ..." would need some more
> rewriting since this patch rather expands the scope of the existing
> flag?

Thanks for pointing this out - the two were separate flags in my original
patch set because I
wanted to minimize the scope of impact; however, I have since received
advice and buy-in
on converging them and RUNTIME_PREFIX_PERL functionality was merged
underneath
of RUNTIME_PREFIX in this latest patch set.

I'll update the commit message!

Re: [PATCH v6 2/3] Makefile: add Perl runtime prefix support

[Daniel Jacques]

On Mon, Mar 19, 2018 at 3:21 PM Ævar Arnfjörð Bjarmason 
wrote:

> I think it would be more idiomatic and more paranoid (we'll catch bugs)
> to do:

>   my $exec_path;
>   if (exists $ENV{GIT_EXEC_PATH}) {
>   $exec_path = $ENV{GIT_EXEC_PATH};
>   } else {
>   [...]
>   }

> I.e. we're interested if we got passed GIT_EXEC_PATH, so let's see if it
> exists in the env hash, and then use it as-is. If we have some bug where
> it's an empty string we'd like to know, presumably...

Good idea, done.

> > +
> > + # Trim off the relative gitexecdir path to get the system path.
> > + (my $prefix = $exec_path) =~ s=${gitexecdir_relative}$==;

> The path could contain regex metacharacters, so let's quote those via:

>   (my $prefix = $exec_path) =~ s/\Q$gitexecdir_relative\E$//;

> This also nicely gets us rid of the more verbose ${} form, which makes
> esnse when we're doing ${foo}$ instead of the arguably less readbale
> $foo$, but when it's \Q$foo\E$ it's clear what's going on.

Ah cool - makes sense. I'm not strong with Perl, so I wasn't aware that
this was an option, but I agree it's cleaner. Done.

Re: [PATCH v6 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Daniel Jacques]

On Mon, Mar 19, 2018 at 3:27 PM Ævar Arnfjörð Bjarmason 
wrote:

> I wonder if it wouldn't be a lot more understandable if these were noted
> together, i.e. let's first document RUNTIME_PREFIX, then for all the
> other ones say below that:

Sounds good to me, done.

> Whitespace changed mixed in with the actual change.

Oops, automatic "clang-format" slipped in there. I've reverted this part.

Thanks for reviewing!

Re: [PATCH v6 2/3] Makefile: add Perl runtime prefix support

[Daniel Jacques]

> > The merge conflict becomes a tad easier to deal with, also makes sense
> > to have gitexecdir after infodir since that's the order we're listing
> > these in just a few lines earlier, and this is otherwise (mostly)
> > consistent.

Actually as a quick follow-up question: for these patch sets, is it best
for me to have them based off of "master", "next", or a different branch?

Re: [PATCH v6 2/3] Makefile: add Perl runtime prefix support

[Daniel Jacques]

On Mon, Mar 19, 2018 at 3:12 PM Ævar Arnfjörð Bjarmason 
wrote:

> The merge conflict becomes a tad easier to deal with, also makes sense
> to have gitexecdir after infodir since that's the order we're listing
> these in just a few lines earlier, and this is otherwise (mostly)
> consistent.

Got it, I'll update my patch set to include this in v7, which I'll post
after a little more time for comment on v6. Thanks!

Re: [PATCH v6 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Daniel Jacques]

On Mon, Mar 19, 2018 at 1:24 PM Junio C Hamano  wrote:

> Look for these misspelled words:

Oh boy ... thanks, and done.

> OK.  An essentially no-op change but with the name better suited in
> the extended context---we used to only care about argv0 but that was
> an implementation detail of "where did our binary come from".  Nice.

Yes, exactly. Plus I think some other patches that I've seen circulating
around here recently use it in this new capacity, so the name update is
appropriate.

Re: [PATCH v6 2/3] Makefile: add Perl runtime prefix support

[Daniel Jacques]

On Mon, Mar 19, 2018 at 1:14 PM Junio C Hamano  wrote:

> > +# 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:

> dependency?

Oops, this is the second typo that has been pointed out. I'll release one
last series after a small review period with these fixed.

> > +# - 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

> I see Dscho is CC'ed so I won't worry about "is there a more
> portable test than 'the path begins with a slash' to see if a path
> is relative, or is this good enough even for Windows in the context
> of this patch?".  It won't be a show-stopper issue as long as we do
> not error out with false positive, though ;-).

OK sounds good! There are other places in the Makefile that use this method
for this purpose, so hopefully the worst-case is that this is no more
broken than they are.

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

[Daniel Jacques]

> It would be great to have this rebooted now that my perl cleanup efforts
> have un-blocked this. Will be happy to help review & test the next
> iteration.

Yes, I was just thinking the same thing. I wanted to make sure the Perl
changes had landed, and I'm pleased to see that they have. I should have
time in the next few days to rebase and put up a new version of the patch
series. I'll keep you in the loop, and thanks for pinging!

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

[Daniel Jacques]

On Wed, Dec 6, 2017 at 1:47 PM, Junio C Hamano  wrote:
>
> Johannes Sixt  writes:
>
> > The updated series works for me now. Nevertheless, I suggest to squash
> > in the following change to protect against IFS and globbing characters in
> > $INSTLIBDIR.
>
> Yeah, that is very sensible.
>
> > diff --git a/Makefile b/Makefile
> > index 7ac4458f11..08c78a1a63 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
> > GIT-PERL-DEFINES perl/perl.mak Makefile
> >   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> >   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && 
> > \
> >   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> > - -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> > + -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
> >   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
> >   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
> >   $< >$@+ && \

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