Marius Bakke writes: > Hi Chris, > > Christopher Baines writes: > >> Christopher Baines 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? Not quite... I think lollypop was reading the typelib in libsoup, but the shared library was just referenced by filename, not the absolute filename, and I think this was causing issues when trying to use libsoup from lollypop. On master: grep shared-library /gnu/store/bafaiiblr2vmmf1zvidkw1137ndqnqg2-libsoup-2.66.2/share/gir-1.0/Soup-2.4.gir shared-library="libsoup-2.4.so.1" On core-updates: grep shared-library /gnu/store/b1ykh6xj11v7zav4r68v8qflk31cnddm-libsoup-2.66.2/share/gir-1.0/Soup-2.4.gir shared-library="/gnu/store/b1ykh6xj11v7zav4r68v8qflk31cnddm-libsoup-2.66.2/lib/libsoup-2.4.so.1" > 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 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? Ah, I didn't realise this part of the patch was as Nix specific as it is... At least for the change I was trying to affect, this seems to be probably redundant, or somehow doing the job. Maybe this part of the patch relating to the fallback_libpath should be removed. >> ++ 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. Given NIX_STORE is set at build time, and this code is mostly used at build time, then that would work. Before I was thinking about how to actually put the store path in the code at build time, but that's probably not necessary. >> ++ 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.