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: 6926 bytes --]


On 24-07-2022 14:16, Sören Tempel wrote:
> Hi,
>
> Thanks for your feedback, comments below.
>
> Maxime Devos<maximedevos@telenet.be>  wrote:
>> 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.
> Sure, that could work too.
>
>> 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.
> What kind of manual patching do you do on Guix? Could you refer me to
> the code for that? Maybe that's something we could also do on Alpine in
> the meantime.

A few examples:

  * guile-g-golf:
    https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/guile-xyz.scm#n2006
  * guile-artanis:
    https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/guile-xyz.scm#n139
  * guile-aspell:
    https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/guile-xyz.scm#n395
  * guile-squeue
    https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/guile-xyz.scm#n870

'guile-squeue' is one of the simplest examples.

>> IIUC, the string-prefix? search is non-deterministic, which can make
>> debugging complicated when multiple versions are installed.
> Well, on Linux readdir(3) file name order depends on the file system
> implementation. Since the algorithm returns the first file with a
> substring match, it depends on the readdir order. As long as the same
> filesystem is used, it should be deterministic.

'File system-specific' is a form of non-determinism. Additionally, the 
file system implementation can change over time, so even if a file 
system is fixed in advance, it would still be non-deterministic.

Even if the file system implementation doesn't change, there can still 
be non-determinism. For example, if directories are implemented as hash 
tables and 'readdir' iterates over the buckets -- if other files are 
added or removed, the size changes, which can cause different hashed to 
be used and hence a different order. As such, the readdir order can 
depend on the presence and absence of other files, which seems an 
undesirable form of non-determinism to me.

>> I think it would be better to bail out if there are multiple matches
>> instead of a risk of guessing incorrectly.

In that situation, they are all the same file, just with a different 
name, so in that situation it's no problem.

However, consider multiple (potentially incompatible) versions. What 
version do you select then? Maybe just the latest? But they might be 
incompatible ...

>>    * 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.
> Yep, it does. I originally didn't want to modify the handling of
> absolute paths but unfortunately during testing I noticed that Guile
> extensions seem to be loaded with an absolute path

I don't see what's unfortunate about that.

> and hence don't work without the libfoo.so symlink [1].

Seems to work locally:

(load-extension "/home/antipode/.guix-home/profile/lib/libpng16.so.16" 
"t") ; In procedure dlsym: Error: ... --- though library was loaded 
succesfully.

It appears that load-extension already automatically adds a .so though 
(tested with (load-extension 
"/home/antipode/.guix-home/profile/lib/libpng16" "t"), so extending it 
to also add the .N as done here doesn't seem a regression to me.

>>    * If doing libfoo.so.N, will it search for libfoo.so.N.M?
> Yes, since libfoo.so.N is a prefix of libfoo.so.N.M.

What about: ‘If doing libfoo.so.5, will it accept libfoo.so.50"? 
libfoo.so.5 is a prefix of libfoo.so.50, so unless care is taken with 
dots, it could accept a different version than was asked for.

>>    * 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.
> If those paths are also searched using the load-foreign-library
> procedure then they are affected by this change. Also, I am not a Guile
> expert but on Alpine, Guile extensions such as guile-reader also ship
> versioned sonames [1].

They are versioned, but AFAIK the versioning is mostly meaningless, as 
(IIRC) the installation directory for Guile extensions is versioned 
(using the Guile version) and as libguile doesn't take care of properly 
changing the version in case of new symbols or incompatible changes.

Though apparently sometimes (e.g. for guile-reader) it's just put in 
[...]/lib ...

>>    * To test it, and avoid breaking things later with future changes to
>>      load-foreign-library, could some tests be added?
> Probably, though I am not familiar with the Guile test setup and there
> don't seem to be any existing tests for foreign-library.

test-suite/tests/foreign.test, though it does not test actually loading 
the libraries, only the Guile equivalent of dlsym.

I always forget how to run individual tests (instead of the whole suite) 
myself.

>>   * 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.)
> In my experience, most languages which don't link against shared
> libraries directly but instead load them at run-time don't hardcode ABI
> versions (for example, refer to Python's ctypes.util.find_library).

How does this show the (un)desirability of the change?

The fact that most languages don't that, doesn't seem relevant to me. 
Guile is not most languages, and the explanation I gave is not specific 
to Guile, it would work just as well for Python.

More generally, you appear to be inferring "X is acceptable" from "many 
entities do X", ignoring my explanation of why "X is not acceptable". 
This is a variant of the "ad populum" fallacy.

> Also, the current implementation of load-foreign-library does not force
> you to specify an ABI version but instead loads whatever the libfoo.so
> symlink refers to.
I have not made any proposal to change this behaviour.

Greetings,
Maxime.

[-- Attachment #1.1.1.2: Type: text/html, Size: 10693 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 --]