Re: [PATCH] load-foreign-library: perform substring match on library files

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3614 bytes --]


On 23-07-2022 13:09, soeren@soeren-tempel.net wrote:
> From: Sören Tempel<soeren@soeren-tempel.net>
>
> This patch is a fix for bug #49232 [1]. To summarize this bug, the
> current load-foreign-library implementation does not load versioned
> sonames (e.g. libfoo.so.5) which are common on Linux. This is an issue
> for us at Alpine Linux since we ship unversioned sonames (e.g. libfoo.so)
> separately. Please refer to the original bug report for details.
>   [...]

Long term, I think it would be ideal for Guile to decide upon a major 
version (and maybe even location, depending on the choices of the 
distro) at _compile_ time instead of runtime, not unlike how other 
compilers work.

More concretely, we could have a macro 'link-foreign-library', of the form:

(link-foreign-library library [maybe other arguments).

If the Guile module is being compiled with --rpath, it searches 
$CROSS_LIBRARY_PATH or $LIBRARY_PATH and encodes the full file name 
(/usr/lib/.../libguile-... or /gnu/store/.../lib/...) in the .go, which 
avoids some manual patching we have to do in Guix.

It could also detect if it's loading a libfoo.so pointing to 
libfoo.so.N, and if so, replace libfoo.so by libfoo.so.N.

The former (rpath) would work for Guix, and I expect the latter 
(libfoo.so -> libfoo.so.N) to work on Alpine.

> +(define (file-exists-in-dir-with-extension dir basename exts)
> +  (let* ((dir-stream (opendir dir))
> +         (ret (let loop ((fn (readdir dir-stream)))
> +                (and (not (eof-object? fn))
> +                     (if (filename-matches-with-extension? fn basename exts)
> +                       (in-vicinity dir fn)
> +                       (loop (readdir dir-stream)))))))
IIUC, the string-prefix? search is non-deterministic, which can make 
debugging complicated when multiple versions are installed. I think it 
would be better to bail out if there are multiple matches instead of a 
risk of guessing incorrectly.

The prefixing behaviour is also not documented, so some documentation is 
needed in the manual.

Questions:

  * Does it go scanning the dir even if libfoo.so could be found?
    Otherwise, there are some possible performance gains by checking for
    libfoo.so first -- consider the case where /usr/lib is huge.
  * When doing (load-foreign-library "/gnu/store/.../libfoo.so")
    (absolute file name!), would it search for
    /gnu/store/.../libfoo.so.N?  If so, that would be surprising,
    especially if libfoo.so.N exists.
  * If doing libfoo.so.N, will it search for libfoo.so.N.M?
  * Does it only apply to the system paths, or also to
    GUILE_SYSTEM_EXTENSION_PATH, LTDL_LIBRARY_PATH and
    GUILE_EXTENSION_PATH? The latter would be surprising to me, as
    versioning is more of a system thing.
  * To test it, and avoid breaking things later with future changes to
    load-foreign-library, could some tests be added?
  * Is this change desirable?  I mean, this is an FFI API, so the ABI of
    the library is rather important. If a Guile module links to
    libfoo.so, and they had version N in mind, then it's important it
    doesn't link to N-1 or N+1 instead, because of ABI
    incompatibilities. As such, to me it seems _good_ that you got some
    errors, as now you get a reminder to explicitly state which ABI
    version is needed. (YMMV, and the mileage of the Guile maintainers
    might vary, etc.)

Also, this seems like a non-trivial change to me, so a copyright line 
might be in order, unless you did the copyright assignment.

Greetings,
Maxime.


[-- Attachment #1.1.1.2: Type: text/html, Size: 4673 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]