From: Junio C Hamano <gitster@pobox•com>
To: Michael Weiser <michael.weiser@gmx•de>
Cc: git@vger•kernel.org, David Abdurachmanov <David.Abdurachmanov@cern•ch>
Subject: Re: [PATCH] Extend runtime prefix computation
Date: Fri, 15 Apr 2016 09:43:29 -0700 [thread overview]
Message-ID: <xmqqfuumddqm.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160415143001.GA67437@dinsnail.net> (Michael Weiser's message of "Fri, 15 Apr 2016 16:30:01 +0200")
Michael Weiser <michael.weiser@gmx•de> writes:
> Make git fully relocatable at runtime extending the runtime prefix
> calculation. Handle absolute and relative paths in argv0. Handle no path
> at all in argv0 in a system-specific manner. Replace assertions with
> initialised variables and checks that lead to fallback to the static
> prefix.
That's a dense description of "what" without saying much about
"why". Hint: start by describing what case(s) the current code
fails to find the correct runtime prefix. That would give readers a
better understanding of what problem you are trying to solve.
Missing sign-off.
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9d5703a..f0db070 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -3,30 +3,27 @@
> #include "quote.h"
> #include "argv-array.h"
> #define MAX_ARGS 32
> +#if defined(__APPLE__)
> +#include <mach-o/dyld.h>
> +#endif
We tend to avoid system specific includes in individual *.c files.
Perhaps implement platform specific bits in compat/? E.g. each
platform specific file in compat/ may implement and export the same
git_extract_argv_path() function, perhaps, so that this file does
not even need to know what platforms it supports?
> char *system_path(const char *path)
> {
> -#ifdef RUNTIME_PREFIX
> - static const char *prefix;
> -#else
> static const char *prefix = PREFIX;
> -#endif
> struct strbuf d = STRBUF_INIT;
>
> if (is_absolute_path(path))
> return xstrdup(path);
>
> #ifdef RUNTIME_PREFIX
> - assert(argv0_path);
> - assert(is_absolute_path(argv0_path));
Aren't these protecting against future and careless change that
forgets to call extract-argv0-path or make that function return
something that is not an absolute path?
Is it a good idea to drop these checks, and if so why?
> - 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"))) {
> + if (!argv0_path ||
> + !is_absolute_path(argv0_path) ||
> + (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
> + !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
> + !(prefix = strip_path_suffix(argv0_path, "git")))) {
> prefix = PREFIX;
> trace_printf("RUNTIME_PREFIX requested, "
> "but prefix computation failed. "
> @@ -41,6 +38,8 @@ char *system_path(const char *path)
> const char *git_extract_argv0_path(const char *argv0)
> {
> const char *slash;
> + char *to_resolve = NULL;
> + const char *resolved;
>
> if (!argv0 || !*argv0)
> return NULL;
> @@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0)
> slash = find_last_dir_sep(argv0);
>
> if (slash) {
> - argv0_path = xstrndup(argv0, slash - argv0);
> - return slash + 1;
> + /* ... it's either an absolute path */
> + if (is_absolute_path(argv0)) {
> + argv0_path = xstrndup(argv0, slash - argv0);
> + return slash + 1;
> + }
> +
> + /* ... or a relative path, in which case we have to make it
> + * absolute first and do the whole thing again */
> + to_resolve = xstrdup(argv0);
> + } else {
> + /* argv0 is no path at all, just a name. Resolve it into a
> + * path. Unfortunately, this gets system specific. */
> +#if defined(__linux__)
> + struct stat st;
> + if (!stat("/proc/self/exe", &st))
> + to_resolve = xstrdup("/proc/self/exe");
> +#elif defined(__APPLE__)
> + /* call with len == 0 queries the necessary buffer size */
> + uint32_t len = 0;
> + if(_NSGetExecutablePath(NULL, &len) != 0) {
> + to_resolve = malloc(len);
> + if (to_resolve) {
> + /* get the executable path now we have a buffer
> + * of proper size */
> + if(_NSGetExecutablePath(to_resolve, &len) != 0) {
> + free(to_resolve);
> + return argv0;
> + }
> + }
> + }
> +#endif
> +
> + /* if to_resolve is still NULL here, something failed above or
> + * we are on an unsupported system. system_path() will warn
> + * and fall back to the static prefix */
> + if (!to_resolve)
> + return argv0;
> }
>
> - return argv0;
> + /* resolve path from absolute to canonical */
> + resolved = real_path(to_resolve);
> + free(to_resolve);
> +
> + slash = find_last_dir_sep(resolved);
> + if (!slash)
> + return argv0;
> +
> + argv0_path = xstrndup(resolved, slash - resolved);
> + slash = xstrdup(slash + 1);
> + return slash;
> }
>
> void git_set_argv_exec_path(const char *exec_path)
next prev parent reply other threads:[~2016-04-15 16:43 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-15 14:30 [PATCH] Extend runtime prefix computation Michael Weiser
2016-04-15 16:43 ` Junio C Hamano [this message]
2016-04-18 7:20 ` Johannes Schindelin
2016-04-20 17:52 ` Michael Weiser
-- strict thread matches above, loose matches on Subject: below --
2012-11-27 16:30 Michael Weiser
2012-11-30 10:20 ` Erik Faye-Lund
2012-11-30 10:45 ` Michael Weiser
2013-03-05 11:58 ` Michael Weiser
2013-03-05 16:13 ` Junio C Hamano
2013-03-06 8:19 ` Michael Weiser
2013-04-16 14:56 ` Michael Weiser
2013-04-16 18:23 ` Junio C Hamano
2013-04-16 15:18 ` Erik Faye-Lund
2013-04-17 6:06 ` Michael Weiser
2013-10-04 13:32 ` Michael Weiser
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqfuumddqm.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=David.Abdurachmanov@cern$(echo .)ch \
--cc=git@vger$(echo .)kernel.org \
--cc=michael.weiser@gmx$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox