exec_cmd: RUNTIME_PREFIX on some POSIX systems
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest1 patch 8 repliesIn review
- v11 patch 1 replySuperseded
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/1 | exec_cmd: RUNTIME_PREFIX on some POSIX systems.gitignore Makefile cache.h +6 more | +304 | 8 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.
Cover letter [PATCH v2 0/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems
Previous round:
- https://public-inbox.org/git/[email protected]/
- https://public-inbox.org/git/[email protected]/
Junio,
Thanks for taking the time to review this patch. Responses inline:
Quoted 5 lines
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.
Quoted 2 lines
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".
Quoted 2 lines
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.
Quoted 4 lines
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 variableargv_exec_pathwe 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.
Quoted 1 line
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!
Quoted 2 lines
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.
Quoted 2 lines
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(-)
Signature · 1 line
2.15.0.448.gf294e3d99a-goog