Subject: Re: bug#75688: Replace wrapper scripts with search path value files in search-paths.d

From: Maxim Cournoyer <[email-masked]>
Date: Tue, 28 Jan 2025 11:41:31 +0900

Hi,

A few additional comments.

[email-masked] writes:

> From: 宋文武 <[email-masked]>
>
> 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@gmail>

-- 
Thanks,
Maxim