[bug#75688,v2,1/4] gnu: glib: Support load search paths from etc/search-paths.d files.

Message ID 916f2d3087c47d6212682656f47a2899ba795df1.1737613671.git.iyzsong@member.fsf.org
State New
Headers
Series [bug#75688,v2,1/4] gnu: glib: Support load search paths from etc/search-paths.d files. |

Commit Message

Alexis Praga via Guix-patches via Jan. 23, 2025, 6:28 a.m. UTC
  From: 宋文武 <iyzsong@member.fsf.org>

Add a new function "g_build_guix_search_path_dirs" to GLIB, which in addition
to environment variable, read search path values from 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.

Change-Id: I1d6d113fc38b20ebd4dce195f6d9c58ce85967e4
---
 gnu/local.mk                                  |   1 +
 gnu/packages/glib.scm                         |   9 +-
 .../patches/glib-guix-search-paths.patch      | 158 ++++++++++++++++++
 3 files changed, 166 insertions(+), 2 deletions(-)
 create mode 100644 gnu/packages/patches/glib-guix-search-paths.patch


base-commit: 7080aaf08102ec4c9c976582d6adfa0c14e6c640
  

Comments

Maxim Cournoyer Jan. 27, 2025, 2:59 a.m. UTC | #1
Hello,

iyzsong@envs.net writes:

> From: 宋文武 <iyzsong@member.fsf.org>
>
> Add a new function "g_build_guix_search_path_dirs" to GLIB, which in addition
> to environment variable, read search path values from etc/search-paths.d

Some typo/grammar nitpicks:

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

nitpick: I'd use a '.  ' instead of ', ' above (two sentences).

[...]

> +++ b/gnu/packages/patches/glib-guix-search-paths.patch
> @@ -0,0 +1,158 @@

I think it'd be nice to forward a subset of this patch that implements
just the loading environment variable from a file, as that mechanism
seems like it could be generally useful (and upstreaming it would lower
the maintenance burden for us).

> +diff --git a/gio/giomodule.c b/gio/giomodule.c
> +index 76c2028..49b02bb 100644
> +--- a/gio/giomodule.c
> ++++ b/gio/giomodule.c
> +@@ -1330,6 +1330,13 @@ _g_io_modules_ensure_loaded (void)
> +       g_io_modules_scan_all_in_directory_with_scope (module_dir, scope);
> +       g_free (module_dir);
> +
> ++      /* GUIX: Load gio modules from GUIX_GIO_EXTRA_MODULES */

Let's not add credence to the Reddit's GUIX misspelling ;-).  I'd drop
'GUIX: ' from the beginning of your comment, as it's already clear from
the variable name.  Please add a period to make it a complete sentence.

[...]

> +diff --git a/glib/gutils.c b/glib/gutils.c
> +index 8628a56..bc21efc 100644

[...]

> ++gchar **
> ++g_build_guix_search_path_dirs (const gchar *variable)

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

I'd ensure all lines wrapped around the 80 characters mark (here and
everywhere else).

> ++  gchar *exe_path = g_file_read_link ("/proc/self/exe", NULL);
> ++  gchar *out_path = NULL;
> ++  gchar *search_paths_d = NULL;
> ++
> ++  /* We install executables under "bin" or "libexec", can also be a subdirectory of "libexec". */
> ++  if (exe_path && (g_str_match_string("/bin/", exe_path, FALSE) ||
> ++                   g_str_match_string("/libexec/", exe_path, FALSE))) {

Perhaps these 'bin' and 'libexec' hard-coded names should come from the
build system of gdk-pixbuf, in case a distro uses different names across
its package collection (to make it more general).

> ++    /* Find output directory, which is the parent directory of "bin" or "libexec". */
> ++    out_path = g_path_get_dirname (exe_path);
> ++    while (g_str_match_string("/bin/", out_path, FALSE) ||
> ++           g_str_match_string("/libexec/", out_path, FALSE)) {
> ++      gchar *dir_path = out_path;

Is the intent above to *copy* out_path into dir_path?  Currently that's
not done; we just point another pointer to it.

> ++      out_path = g_path_get_dirname (dir_path);

If g_path_get_dirname mutates dir_path, than dir_path should be a string
copy.  Otherwise if it doesn't get mutated by the call, we should be
able to use just:

out_path = g_path_get_dirname (out_path);

> ++      g_free (dir_path);
> ++    }
> ++
> ++    /* Now add paths from etc/search-paths.d/VARIABLE file. */
> ++    search_paths_d = g_build_filename (out_path, "etc", "search-paths.d", NULL);
> ++    if (g_file_test (search_paths_d, G_FILE_TEST_EXISTS)) {
> ++      gchar *var_path = g_build_filename (search_paths_d, variable, NULL);
> ++      if (g_file_get_contents (var_path, &value, NULL, NULL)) {
> ++        dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
> ++        g_strv_builder_addv (builder, (const gchar **) dirs);
> ++        g_strfreev (dirs);
> ++        g_free (value);
> ++      }
> ++      g_free (var_path);
> ++    }
> ++  }
> ++
> ++  g_free (exe_path);
> ++  g_free (out_path);
> ++  g_free (search_paths_d);
> ++#endif
> ++
> ++  /* Then add paths from the environment variable. */
> ++  gboolean is_setuid = GLIB_PRIVATE_CALL (g_check_setuid) ();
> ++  if (is_setuid) /* we don't want to access arbitrary files when running as setuid. */
> ++    value = NULL;
> ++  else
> ++    value = g_strdup (g_getenv (variable));
> ++
> ++  if (value && value[0]) {
> ++      dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
> ++      g_strv_builder_addv (builder, (const gchar **) dirs);
> ++      g_strfreev (dirs);
> ++  }
> ++  g_free (value);
> ++
> ++  dirs = g_strv_builder_end (builder);
> ++  g_strv_builder_unref (builder);
> ++  return dirs;
> ++}

Apart from my above comments, this looks good to me.  I think I'd stress
once more the value of upstreaming as much of this to ease maintenance
in the future.

Thanks for this novel idea/implementation!
  
宋文武 Jan. 27, 2025, 5:17 a.m. UTC | #2
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>> +++ b/gnu/packages/patches/glib-guix-search-paths.patch
>> @@ -0,0 +1,158 @@
>
> I think it'd be nice to forward a subset of this patch that implements
> just the loading environment variable from a file, as that mechanism
> seems like it could be generally useful (and upstreaming it would lower
> the maintenance burden for us).

Okay, I could try.


>> ++  gchar *exe_path = g_file_read_link ("/proc/self/exe", NULL);

While, I just find that this "/proc/self/exe" alone only works for ELF
executables, for interpreted scripts, we also need patch interpreters to
set 2 environment variables, eg: GUIX_INTERPRETER_PATH and GUIX_MAIN_SCRIPT_PATH.

And use GUIX_MAIN_SCRIPT_PATH when "/proc/self/exe" match
GUIX_INTERPRETER_PATH...

I'll send updated patches later.


>> ++  gchar *out_path = NULL;
>> ++  gchar *search_paths_d = NULL;
>> ++
>> ++  /* We install executables under "bin" or "libexec", can also be a subdirectory of "libexec". */
>> ++  if (exe_path && (g_str_match_string("/bin/", exe_path, FALSE) ||
>> ++                   g_str_match_string("/libexec/", exe_path, FALSE))) {
>
> Perhaps these 'bin' and 'libexec' hard-coded names should come from the
> build system of gdk-pixbuf, in case a distro uses different names across
> its package collection (to make it more general).

Okay, I could do this when upstream.

>> ++    /* Find output directory, which is the parent directory of "bin" or "libexec". */
>> ++    out_path = g_path_get_dirname (exe_path);
>> ++    while (g_str_match_string("/bin/", out_path, FALSE) ||
>> ++           g_str_match_string("/libexec/", out_path, FALSE)) {
>> ++      gchar *dir_path = out_path;
>
> Is the intent above to *copy* out_path into dir_path?  Currently that's
> not done; we just point another pointer to it.
>
>> ++      out_path = g_path_get_dirname (dir_path);
>
> If g_path_get_dirname mutates dir_path, than dir_path should be a string
> copy.  Otherwise if it doesn't get mutated by the call, we should be
> able to use just:
>
> out_path = g_path_get_dirname (out_path);
>
>> ++      g_free (dir_path);

That 'dir_path' is only maded to be freed here, since
'g_path_get_dirname' allocate a new array instead of modify existed one,
so we need free the old out_path once we get a new one.


> [...]
>
> Apart from my above comments, this looks good to me.  I think I'd stress
> once more the value of upstreaming as much of this to ease maintenance
> in the future.
>
> Thanks for this novel idea/implementation!

Sure, with additional interpreter patches which I haven't sent, to be
honest I feel this more hacky than novel, hope upstream or other folks
can give better ideas...

Thank you!
  
Maxim Cournoyer Jan. 28, 2025, 1:15 p.m. UTC | #3
Hello,

宋文武 <iyzsong@envs.net> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>>> +++ b/gnu/packages/patches/glib-guix-search-paths.patch
>>> @@ -0,0 +1,158 @@
>>
>> I think it'd be nice to forward a subset of this patch that implements
>> just the loading environment variable from a file, as that mechanism
>> seems like it could be generally useful (and upstreaming it would lower
>> the maintenance burden for us).
>
> Okay, I could try.

Seems upstream prefer we open an issue to discuss the use case/feature
request first.  I've done so for the proposed GDK_PIXBUF_MODULE_FILES
environment variable here [0].

[0]  https://gitlab.gnome.org/GNOME/gdk-pixbuf/-/issues/247

[...]

>>> ++    /* Find output directory, which is the parent directory of "bin" or "libexec". */
>>> ++    out_path = g_path_get_dirname (exe_path);
>>> ++    while (g_str_match_string("/bin/", out_path, FALSE) ||
>>> ++           g_str_match_string("/libexec/", out_path, FALSE)) {
>>> ++      gchar *dir_path = out_path;
>>
>> Is the intent above to *copy* out_path into dir_path?  Currently that's
>> not done; we just point another pointer to it.
>>
>>> ++      out_path = g_path_get_dirname (dir_path);
>>
>> If g_path_get_dirname mutates dir_path, than dir_path should be a string
>> copy.  Otherwise if it doesn't get mutated by the call, we should be
>> able to use just:
>>
>> out_path = g_path_get_dirname (out_path);
>>
>>> ++      g_free (dir_path);
>
> That 'dir_path' is only maded to be freed here, since
> 'g_path_get_dirname' allocate a new array instead of modify existed one,
> so we need free the old out_path once we get a new one.

Thanks for the explanation.

>> [...]
>>
>> Apart from my above comments, this looks good to me.  I think I'd stress
>> once more the value of upstreaming as much of this to ease maintenance
>> in the future.
>>
>> Thanks for this novel idea/implementation!
>
> Sure, with additional interpreter patches which I haven't sent, to be
> honest I feel this more hacky than novel, hope upstream or other folks
> can give better ideas...

It's indeed a bit hacky, but still an improvement over wrapping
everything with leaky shell scripts in my opinion.

--
Thanks,
Maxim
  

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 5091f93eb8..6bb3a35770 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1436,6 +1436,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/git-filter-repo-generate-doc.patch	\
   %D%/packages/patches/gklib-suitesparse.patch			\
   %D%/packages/patches/glib-appinfo-watch.patch			\
+  %D%/packages/patches/glib-guix-search-paths.patch		\
   %D%/packages/patches/glib-skip-failing-test.patch		\
   %D%/packages/patches/glibc-2.33-riscv64-miscompilation.patch	\
   %D%/packages/patches/glibc-2.39-git-updates.patch	\
diff --git a/gnu/packages/glib.scm b/gnu/packages/glib.scm
index e04eedb7ba..0704ba2c53 100644
--- a/gnu/packages/glib.scm
+++ b/gnu/packages/glib.scm
@@ -258,7 +258,8 @@  (define glib
         (base32 "0c3vagxl77wma85qinbj974jvw96n5bvch2m7hqcwxq8fa5spsj4"))
        (patches
         (search-patches "glib-appinfo-watch.patch"
-                        "glib-skip-failing-test.patch"))
+                        "glib-skip-failing-test.patch"
+                        "glib-guix-search-paths.patch"))
        (modules '((guix build utils)))
        (snippet
         '(begin
@@ -516,9 +517,13 @@  (define glib
       (search-path-specification
        (variable "XDG_DATA_DIRS")
        (files '("share")))
+      ;; To load gsettings schemas from GTK, etc.
+      (search-path-specification
+       (variable "GUIX_GSETTINGS_SCHEMA_DIR")
+       (files '("share/glib-2.0/schemas")))
       ;; To load extra gio modules from glib-networking, etc.
       (search-path-specification
-       (variable "GIO_EXTRA_MODULES")
+       (variable "GUIX_GIO_EXTRA_MODULES")
        (files '("lib/gio/modules")))))
     (search-paths native-search-paths)
     (synopsis "Low-level core library for GNOME projects")
diff --git a/gnu/packages/patches/glib-guix-search-paths.patch b/gnu/packages/patches/glib-guix-search-paths.patch
new file mode 100644
index 0000000000..ebd57024dd
--- /dev/null
+++ b/gnu/packages/patches/glib-guix-search-paths.patch
@@ -0,0 +1,158 @@ 
+diff --git a/gio/giomodule.c b/gio/giomodule.c
+index 76c2028..49b02bb 100644
+--- a/gio/giomodule.c
++++ b/gio/giomodule.c
+@@ -1330,6 +1330,13 @@ _g_io_modules_ensure_loaded (void)
+       g_io_modules_scan_all_in_directory_with_scope (module_dir, scope);
+       g_free (module_dir);
+ 
++      /* GUIX: Load gio modules from GUIX_GIO_EXTRA_MODULES */
++      gchar **guix_giomodule_dirs = g_build_guix_search_path_dirs ("GUIX_GIO_EXTRA_MODULES");
++      for (int i = 0; guix_giomodule_dirs[i] != NULL; i++) {
++        g_io_modules_scan_all_in_directory_with_scope (guix_giomodule_dirs[i], scope);
++      }
++      g_strfreev (guix_giomodule_dirs);
++
+       g_io_module_scope_free (scope);
+ 
+       /* Initialize types from built-in "modules" */
+diff --git a/gio/gsettingsschema.c b/gio/gsettingsschema.c
+index e8ccc8c..b8bae14 100644
+--- a/gio/gsettingsschema.c
++++ b/gio/gsettingsschema.c
+@@ -354,6 +354,13 @@ initialise_schema_sources (void)
+       while (i--)
+         try_prepend_data_dir (dirs[i]);
+ 
++      /* GUIX: Load schemas from GUIX_GSETTINGS_SCHEMA_DIR. */
++      char **guix_schema_dirs = g_build_guix_search_path_dirs ("GUIX_GSETTINGS_SCHEMA_DIR");
++      i = g_strv_length(guix_schema_dirs);
++      while (i--)
++        try_prepend_dir (guix_schema_dirs[i]);
++      g_strfreev (guix_schema_dirs);
++
+       try_prepend_data_dir (g_get_user_data_dir ());
+ 
+       /* Disallow loading extra schemas if running as setuid, as that could
+diff --git a/glib/gutils.c b/glib/gutils.c
+index 8628a56..bc21efc 100644
+--- a/glib/gutils.c
++++ b/glib/gutils.c
+@@ -2708,6 +2708,16 @@ g_build_system_data_dirs (void)
+     data_dir_vector = g_strsplit (data_dirs, G_SEARCHPATH_SEPARATOR_S, 0);
+ #endif
+ 
++  /* GUIX: Use data files from GUIX_XDG_DATA_DIRS. */
++  GStrvBuilder *builder = g_strv_builder_new ();
++  g_strv_builder_addv (builder, (const gchar **) data_dir_vector);
++  g_strfreev (data_dir_vector);
++  data_dir_vector = g_build_guix_search_path_dirs ("GUIX_XDG_DATA_DIRS");
++  g_strv_builder_addv (builder, (const gchar **) data_dir_vector);
++  g_strfreev (data_dir_vector);
++  data_dir_vector = g_strv_builder_end (builder);
++  g_strv_builder_unref (builder);
++
+   return g_steal_pointer (&data_dir_vector);
+ }
+ 
+@@ -2800,6 +2810,16 @@ g_build_system_config_dirs (void)
+   conf_dir_vector = g_strsplit (conf_dirs, G_SEARCHPATH_SEPARATOR_S, 0);
+ #endif
+ 
++  /* GUIX: Use config files from GUIX_XDG_CONFIG_DIRS. */
++  GStrvBuilder *builder = g_strv_builder_new ();
++  g_strv_builder_addv (builder, (const gchar **) conf_dir_vector);
++  g_strfreev (conf_dir_vector);
++  conf_dir_vector = g_build_guix_search_path_dirs ("GUIX_XDG_CONFIG_DIRS");
++  g_strv_builder_addv (builder, (const gchar **) conf_dir_vector);
++  g_strfreev (conf_dir_vector);
++  conf_dir_vector = g_strv_builder_end (builder);
++  g_strv_builder_unref (builder);
++
+   return g_steal_pointer (&conf_dir_vector);
+ }
+ 
+@@ -2849,6 +2869,69 @@ g_get_system_config_dirs (void)
+   return system_config_dirs;
+ }
+ 
++gchar **
++g_build_guix_search_path_dirs (const gchar *variable)
++{
++  gchar **dirs = NULL;
++  char *value = NULL;
++  GStrvBuilder *builder = g_strv_builder_new ();
++
++#if defined(__linux__) || defined(__gnu_hurd__)
++  /* First add paths from the etc/search-paths.d, which can be used to replace wrapper script. */
++  gchar *exe_path = g_file_read_link ("/proc/self/exe", NULL);
++  gchar *out_path = NULL;
++  gchar *search_paths_d = NULL;
++
++  /* We install executables under "bin" or "libexec", can also be a subdirectory 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". */
++    out_path = g_path_get_dirname (exe_path);
++    while (g_str_match_string("/bin/", out_path, FALSE) ||
++           g_str_match_string("/libexec/", out_path, FALSE)) {
++      gchar *dir_path = out_path;
++      out_path = g_path_get_dirname (dir_path);
++      g_free (dir_path);
++    }
++
++    /* Now add paths from etc/search-paths.d/VARIABLE file. */
++    search_paths_d = g_build_filename (out_path, "etc", "search-paths.d", NULL);
++    if (g_file_test (search_paths_d, G_FILE_TEST_EXISTS)) {
++      gchar *var_path = g_build_filename (search_paths_d, variable, NULL);
++      if (g_file_get_contents (var_path, &value, NULL, NULL)) {
++        dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
++        g_strv_builder_addv (builder, (const gchar **) dirs);
++        g_strfreev (dirs);
++        g_free (value);
++      }
++      g_free (var_path);
++    }
++  }
++
++  g_free (exe_path);
++  g_free (out_path);
++  g_free (search_paths_d);
++#endif
++
++  /* Then add paths from the environment variable. */
++  gboolean is_setuid = GLIB_PRIVATE_CALL (g_check_setuid) ();
++  if (is_setuid) /* we don't want to access arbitrary files when running as setuid. */
++    value = NULL;
++  else
++    value = g_strdup (g_getenv (variable));
++
++  if (value && value[0]) {
++      dirs = g_strsplit (value, G_SEARCHPATH_SEPARATOR_S, 0);
++      g_strv_builder_addv (builder, (const gchar **) dirs);
++      g_strfreev (dirs);
++  }
++  g_free (value);
++
++  dirs = g_strv_builder_end (builder);
++  g_strv_builder_unref (builder);
++  return dirs;
++}
++
+ /**
+  * g_nullify_pointer:
+  * @nullify_location: (not nullable): the memory address of the pointer.
+diff --git a/glib/gutils.h b/glib/gutils.h
+index efc6914..710cf27 100644
+--- a/glib/gutils.h
++++ b/glib/gutils.h
+@@ -36,6 +36,9 @@
+ 
+ G_BEGIN_DECLS
+ 
++GLIB_AVAILABLE_IN_ALL
++gchar **g_build_guix_search_path_dirs (const gchar *variable);
++
+ GLIB_AVAILABLE_IN_ALL
+ const gchar *         g_get_user_name        (void);
+ GLIB_AVAILABLE_IN_ALL