git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Dan Jacques <dnj@google.com>
To: git@vger.kernel.org
Cc: Dan Jacques <dnj@google.com>
Subject: [PATCH v2 0/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems
Date: Sun, 19 Nov 2017 12:31:40 -0500	[thread overview]
Message-ID: <20171119173141.4896-1-dnj@google.com> (raw)

Previous round:
- https://public-inbox.org/git/20171116170523.28696-1-dnj@google.com/
- https://public-inbox.org/git/20171116170523.28696-2-dnj@google.com/

Junio,

Thanks for taking the time to review this patch. Responses inline:

> The "regardless of whether the user has overridden it" part sounded
> alarming and made me wince twice.  I think you meant...
>
> As I have multiple installations of various versions of Git, I find
> the latter somewhat disturbing.

That's an excellent point. You also raised other concerns farther down
with respect to PERL and gettext() environment variables having a similar
behavior of overriding explicit user values. I had regarded these
environment variables as internal, but if they are part of the user
interface then forcefully overriding them is probably unacceptable.

I took some time and restructured the patch to avoid leveraging
environment variables:

- For Git code, each ancillary tool will now just resolve its own
  executable path.
- For PERL code, this patch introduces "RUNTIME_PREFIX_PERL", which
  injects an alternative header into PERL tooling enabling it to resolve
  against its installation path.
- gettext() now resolves using system_path(), as it should have in the
  first place.

The net result of this is that this patch should no longer presume
ownership of environment variables, nor modify them any differently
than Git always has.

The PERL change is more significant now, so I defined a
"RUNTIME_PREFIX_PERL" flag that makes PERL specifically relocatable. I
moved the behavior of forcefullyl setting NO_PERL_MAKEMAKER from
RUNTIME_PREFIX (used by Git-for-Windows) to RUNTIME_PREFIX_PERL, so this
change should change how Git-for-Windows handles PERL anymore.

> We usually frown upon these because they often gets distracting, but
> I didn't find the ones in this patch too bad.

My apologies - I was emboldened by this line in "CodingGuidelines":

  "Fixing style violations while working on a real change as a
  preparatory clean-up step is good, but otherwise avoid useless code
  churn for the sake of conforming to the style."

I suppose "preparatory" probably means "in a preceding separate commit".

> I presume that this is because we may need to know where to find the
> locale stuff before calling git_setup_gettext(); makes sense.

Correct.

> OK, so that is a more appropriate name for the variable that is a
> logical successor of argv0_path.  I wonder if the file-scope static
> variable argv_exec_path we see above would want to move to somewhere
> closer to one of these "platform specific methods", though.

Looking into this, I realized that the method-local static
"cached_exec_path" variable and the global "argv_exec_path" are
redundant, and that "argv_exec_path" escaped my renaming sweep. I
consolidated the two and moved the new global, "exec_path_value",
closer to the only two methods that reference it.

> I think this is our first use of realpath(), which is XSI.

While looking for examples in the code, I ran into `strbuf_realpath`,
which is a Git-native implementation of "realpath". I switched over to
this, making "procfs" resolution more idiomatic and also slightly
simpler, and this patch no longer uses a new API!

> I wonder why argv0 (i.e. the full-path case) is not
> the first one to try, though---isn't that one the simplest?

I briefly covered this in the cover letter, but "argv[0]" is not reliable
on most (POSIX) systems, since "argv" is a user-supplied value to the
`execve` system call that launches a process. In many cases, "argv[0]"
will be the path of the executable, but in some it will just be the name
of the executable (shells do this when executing via PATH resolution)
and nothing is stopping an `execve` caller from supplying a completely
arbitrary value.

On Windows, "argv[0]" seems to always be the full path of the binary,
and this is something that Git-for-Windows relies on; however, on POSIX,
it is an unreliable source of information. The resolution sled falls
back onto it if all else fails, but strongly prefers a more
authoritative method when avaliable.

I've added some documentation to this effect to hopefully clarify the
rationale in the code.

> Also, I wonder if this caller gets simpler to read and understand if
> each of these "platform specific" ones are done like so[...]

I considered your idea, and while it does declutter the sled code, I
think it has a net detrimental effect because it also removes the
platform-specific context from the sled code: A cursory read of that
function would suggest that every resolution method is attempted on all
platforms, and I think that this is the wrong impression to give.

I've added some comments and formatting around that code to hopefully
address the clutter. I'm not opposed to restructuring it (e.g., use a
resolution function vector), but I thought I'd try formatting first.
Let me know what you think!

===

Changes in "v1" from previous version:

- 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 (1):
  exec_cmd: RUNTIME_PREFIX on some POSIX systems

 .gitignore       |   1 +
 Makefile         |  88 +++++++++++++++++---
 cache.h          |   1 +
 common-main.c    |   4 +-
 config.mak.uname |   7 ++
 exec_cmd.c       | 239 +++++++++++++++++++++++++++++++++++++++++++++++--------
 exec_cmd.h       |   4 +-
 gettext.c        |   8 +-
 git.c            |   2 +-
 9 files changed, 304 insertions(+), 50 deletions(-)

-- 
2.15.0.448.gf294e3d99a-goog


             reply	other threads:[~2017-11-19 17:32 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-19 17:31 Dan Jacques [this message]
2017-11-19 17:31 ` [PATCH 1/1] " Dan Jacques
2017-11-20  1:01   ` Junio C Hamano
2017-11-20 21:00   ` Ævar Arnfjörð Bjarmason
2017-11-20 21:50     ` Ævar Arnfjörð Bjarmason
2017-11-21  2:41       ` Re " Dan Jacques
2017-11-22 12:09         ` Ævar Arnfjörð Bjarmason
2017-11-22 14:27           ` Dan Jacques
2017-11-20 21:58     ` Re(2): " Dan Jacques
2017-11-20 22:46       ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171119173141.4896-1-dnj@google.com \
    --to=dnj@google.com \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH v2 0/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git