GNU bug report logs - #75688
Replace wrapper scripts with search path value files in search-paths.d

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: iyzsong <at> envs.net
Cc: 75688 <at> debbugs.gnu.org, 宋文武 <iyzsong <at> member.fsf.org>, Vivien Kraus <vivien <at> planete-kraus.eu>, Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Subject: [bug#75688] Replace wrapper scripts with search path value files in search-paths.d
Date: Tue, 28 Jan 2025 11:41:31 +0900

Hi,

A few additional comments.

iyzsong <at> envs.net writes:

> From: 宋文武 <iyzsong <at> member.fsf.org>
>
> Add a new function "g_build_guix_search_path_dirs" to GLIB, which in addition

nitpick: It's spelled GLib, not GLIB.

> to environment variables, reads search path values from the etc/search-paths.d
> directory of the current executable.  This can be used to replace wrapper
> scripts.
>
> Use it for GUIX_GSETTINGS_SCHEMA_DIR, GUIX_GIO_EXTRA_MODULES,
> GUIX_XDG_DATA_DIRS and GUIX_XDG_CONFIG_DIRS.
>
> * gnu/packages/patches/glib-guix-search-paths.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Register it.
> * gnu/packages/glib.scm (glib)[source]: Add patch.
> [native-search-paths]: Add GUIX_GSETTINGS_SCHEMA_DIR.  Replace
> GIO_EXTRA_MODULES with GUIX_GIO_EXTRA_MODULES.

Sounds good.

[...]

> ++gchar **
> ++g_build_guix_search_path_dirs (const gchar *variable)
> ++{
> ++  gchar **dirs = NULL;
> ++  char *value = NULL;
> ++  GStrvBuilder *builder = g_strv_builder_new ();
> ++
> ++  /* First add paths from the etc/search-paths.d, which can be used to replace wrapper script. */

Please wrap at 80 chars width, here and elsewhere.

> ++  gchar *out_path = NULL;
> ++  gchar *search_paths_d = NULL;
> ++  gchar *exe_path = g_file_read_link ("/proc/self/exe", NULL);
> ++
> ++  /* For scripts, we use GUIX_MAIN_SCRIPT_PATH to find its location. */
> ++  if (g_strcmp0 (exe_path, g_getenv ("GUIX_INTERPRETER_PATH")) == 0) {
> ++    g_free (exe_path);
> ++    exe_path = g_getenv ("GUIX_MAIN_SCRIPT_PATH");
> ++  }

I don't have an opinion yet about GUIX_MAIN_SCRIPT_PATH and
GUIX_INTERPRETER_PATH, defined in the next patch of this series.  I'll
comment there.

> ++  /* We install executables under "bin" or "libexec", can also be a subdirectory of "libexec". */

nitpick: I'd reword to something like:

Executables are installed under the bin or libexec prefix; they may also
be found in a sub-directory of libexec.

> ++  if (exe_path && (g_str_match_string("/bin/", exe_path, FALSE) ||
> ++                   g_str_match_string("/libexec/", exe_path, FALSE))) {
> ++    /* Find output directory, which is the parent directory of "bin" or "libexec". */

nitpick: Find *the* output

The rest still LGTM.

Reviewed-by: Maxim Cournoyer <maxim.cournoyer <at> gmail>

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.