Subject: Re: bug#36535: [PATCH] gnu: gobject-introspection: Update absolute-shlib-path.patch.

From: Marius Bakke <[email-masked]>
Date: Mon, 08 Jul 2019 16:21:58 +0200

Hi Chris,

Christopher Baines <[email-masked]> writes:

> Christopher Baines <[email-masked]> writes:
>
>> Incorporate some changes from nixpkgs to the gobject-introspection package
>> patches.  This is motivated by looking at issues with libsoup and lollypop.
>> This changes means that the share/gir-1.0/Soup-2.4.gir file within libsoup
>> references libsoup-2.4.so.1 with an absolute filename, whereas previously, the
>> filename wasn't absolute.
>>
>> * gnu/packages/patches/gobject-introspection-absolute-shlib-path.patch:
>> Incorporate changes from nixpkgs.
>> ---
>>  ...ct-introspection-absolute-shlib-path.patch | 141 +++++++++++++++++-
>>  1 file changed, 137 insertions(+), 4 deletions(-)
>>
>
> I've pushed this as [1] to core-updates now, as I wanted to get it in
> before the freeze.

Thank you for addressing this.  IIUC previously lollypop failed to
retain a reference to libsoup-2.4.so.1, whereas with this patch it does?

A few comments about the patch:

> diff --git a/gnu/packages/patches/gobject-introspection-absolute-shlib-path.patch b/gnu/packages/patches/gobject-introspection-absolute-shlib-path.patch
> index d00cc5a420..3c0bb1c6cf 100644
> --- a/gnu/packages/patches/gobject-introspection-absolute-shlib-path.patch
> +++ b/gnu/packages/patches/gobject-introspection-absolute-shlib-path.patch
> @@ -2,10 +2,131 @@
>  # add the full path.
>  #
>  # This patch was provided by Luca Bruno <[email-masked]>  for 
> -# 'gobject-introspection' 1.40.0 in Nix. 
> ---- ./giscanner/utils.py.orig	2014-08-14 22:05:05.055334080 +0200
> -+++ ./giscanner/utils.py	2014-08-14 22:05:24.687497334 +0200
> -@@ -110,17 +110,11 @@
> +# 'gobject-introspection' 1.40.0 in Nix.
> +#
> +# It has since been updated to work with newer versions of
> +# gobject-introspection.
> +--- a/giscanner/scannermain.py
> ++++ b/giscanner/scannermain.py
> +@@ -95,6 +95,39 @@ def get_windows_option_group(parser):
> +     return group
> + 
> + 
> ++def _get_default_fallback_libpath():
> ++    # Newer multiple-output-optimized stdenv has an environment variable
> ++    # $outputLib which in turn specifies another variable which then is used as
> ++    # the destination for the library contents (${!outputLib}/lib).
> ++    store_path = os.environ.get(os.environ.get("outputLib")) if "outputLib" in os.environ else None
> ++    if store_path is None:
> ++        outputs = os.environ.get("outputs", "out").split()

gnu-build-system does not currently export an "outputs" variable.
Perhaps it should?

> ++        if "lib" in outputs:
> ++            # For multiple output derivations let's try whether there is a $lib
> ++            # environment variable and use that as the base store path.
> ++            store_path = os.environ.get("lib")
> ++        elif "out" in outputs:
> ++            # Otherwise we have a single output derivation, so the libraries
> ++            # most certainly will end up in "$out/lib".
> ++            store_path = os.environ.get("out")

Consequently, this is the only ever matching case, and "lib" outputs are
ignored, counter to what one might expect from glancing over this patch.

That is, unless one sets an "outputs" or "outputLib" variable in a
package recipe, so maybe we don't have to do anything here.

> ++
> ++    if store_path is not None:
> ++        # Even if we have a $lib as output, there still should be a $lib/lib
> ++        # directory.
> ++        return os.path.join(store_path, 'lib')
> ++    else:
> ++        # If we haven't found a possible scenario, let's return an empty string
> ++        # so that the shared library won't be prepended with a path.
> ++        #
> ++        # Note that this doesn't mean that all hope is lost, because after all
> ++        # we can still use --fallback-library-path to set one.
> ++        #
> ++        # Also, we're not returning None, because that would make it very
> ++        # difficult to disable adding fallback paths altogether using something
> ++        # like: --fallback-library-path=""
> ++        return ""
> ++
> ++
> + def _get_option_parser():
> +     parser = optparse.OptionParser('%prog [options] sources',
> +                                    version='%prog ' + giscanner.__version__)
> +@@ -205,6 +238,10 @@ match the namespace prefix.""")
> +     parser.add_option("", "--filelist",
> +                       action="store", dest="filelist", default=[],
> +                       help="file containing headers and sources to be scanned")
> ++    parser.add_option("", "--fallback-library-path",
> ++                      action="store", dest="fallback_libpath",
> ++                      default=_get_default_fallback_libpath(),
> ++                      help="Path to prepend to unknown shared libraries")
> + 
> +     group = get_preprocessor_option_group(parser)
> +     parser.add_option_group(group)
> +--- a/giscanner/shlibs.py
> ++++ b/giscanner/shlibs.py
> +@@ -57,6 +57,12 @@ def _ldd_library_pattern(library_name):
> +     $""" % re.escape(library_name), re.VERBOSE)
> + 
> + 
> ++def _ldd_library_guix_pattern(library_name):
> ++    store_dir = re.escape('/gnu/store')

Here we should use:

 os.environ.get("NIX_STORE") if "NIX_STORE" in os.environ else "/gnu/store"

So that it works for non-default store prefixes.

> ++    pattern = r'(%s(?:/[^/]*)+lib%s[^A-Za-z0-9_-][^\s\(\)]*)'
> ++    return re.compile(pattern % (store_dir, re.escape(library_name)))
> ++
> ++
> + # This is a what we do for non-la files. We assume that we are on an
> + # ELF-like system where ldd exists and the soname extracted with ldd is
> + # a filename that can be opened with dlopen().
> +@@ -106,7 +112,8 @@ def _resolve_non_libtool(options, binary, libraries):
> +             output = output.decode("utf-8", "replace")
> + 
> +         shlibs = resolve_from_ldd_output(libraries, output)
> +-        return list(map(sanitize_shlib_path, shlibs))
> ++        fallback_libpath = options.fallback_libpath or "";
> ++        return list(map(lambda p: os.path.join(fallback_libpath, p), map(sanitize_shlib_path, shlibs)))
> + 
> + 
> + def sanitize_shlib_path(lib):
> +@@ -115,19 +122,18 @@ def sanitize_shlib_path(lib):
> +     # In case we get relative paths on macOS (like @rpath) then we fall
> +     # back to the basename as well:
> +     # https://gitlab.gnome.org/GNOME/gobject-introspection/issues/222
> +-    if sys.platform == "darwin":
> +-        if not os.path.isabs(lib):
> +-            return os.path.basename(lib)
> +-        return lib
> +-    else:
> ++
> ++    # Always use absolute paths if available
> ++    if not os.path.isabs(lib):
> +         return os.path.basename(lib)
> ++    return lib
> + 
> + 
> + def resolve_from_ldd_output(libraries, output):
> +     patterns = {}
> +     for library in libraries:
> +         if not os.path.isfile(library):
> +-            patterns[library] = _ldd_library_pattern(library)
> ++            patterns[library] = (_ldd_library_pattern(library), _ldd_library_guix_pattern(library))
> +     if len(patterns) == 0:
> +         return []
> + 
> +@@ -139,8 +145,11 @@ def resolve_from_ldd_output(libraries, output):
> +         if line.endswith(':'):
> +             continue
> +         for word in line.split():
> +-            for library, pattern in patterns.items():
> +-                m = pattern.match(word)
> ++            for library, (pattern, guix_pattern) in patterns.items():
> ++                if line.find('/gnu/store') != -1:

Use $NIX_STORE here, too.

Other than that LGTM.