Support loading llvmlite from the main executable. #829

folded · 2022-03-25T12:32:04Z

Change library loading code to also check for the presence of llvmlite
statically linked into the main executable if the dynamic library fails
to load. This allows llvmlite to be used in hermetic and embedded
build cases.

Check that the version function can be dynamically resolved before
declaring success.

Exit the resource context manager correctly so that any extracted
files are unlinked correctly.

On failure, raise OSError with the first error chained.

Change library loading code to also check for the presence of a statically linked version of llvmlite if the dynamic library fails to load. This allows llvmlite to be used in hermetic and embedded build cases. Check that the version function can be dynamically resolved before declaring success. Exit the resource context manager correctly so that any extracted files are unlinked correctly. On failure, raise OSError with the first error chained.

esc · 2022-03-27T15:05:52Z

Change library loading code to also check for the presence of a statically linked version of llvmlite if the dynamic library fails to load. This allows llvmlite to be used in hermetic and embedded build cases.

Thank you for adding this. There are a few ways to build llvmlite. By default the Numba team will statically link LLVM in both wheels and conda packages. See also:

https://llvmlite.readthedocs.io/en/latest/admin-guide/install.html#pre-built-binaries

Hence I am a little confused as to what benefit this PR brings. May I kindly request, that you describe in slightly more detail: what problems did you encounter and how does this help with the build? Thank you.

folded · 2022-03-28T09:06:33Z

I modified the PR description slightly to make the intention hopefully more clear.

This CL addresses the case where you link llvmlite (and llvm) directly into the main executable. You might do this, for example, if you're using an embedded python runtime and don't want to distribute lots of additional shared libraries. It's admittedly a niche use-case (although an important one for us).

I'm happy to maintain this patch downstream if you feel like it's not sufficiently general,. but I do think there are some other small quality of life improvements in this code and it would be pretty easy to just drop the contextlib.nullcontext(None) that supports looking up the symbols in the main executable.

esc · 2022-03-28T10:52:04Z

I modified the PR description slightly to make the intention hopefully more clear.

This CL addresses the case where you link llvmlite (and llvm) directly into the main executable. You might do this, for example, if you're using an embedded python runtime and don't want to distribute lots of additional shared libraries. It's admittedly a niche use-case (although an important one for us).

I'm happy to maintain this patch downstream if you feel like it's not sufficiently general,. but I do think there are some other small quality of life improvements in this code and it would be pretty easy to just drop the contextlib.nullcontext(None) that supports looking up the symbols in the main executable.

OK, if this patch has value for you I don't mind considering it. It's just important that this does not impact what is currently working. Also, would you be open to adding more comments to the source code? There are a few lines where it isn't immediately obvious to me what is going on and they could IMHO do with some additional comments.

folded · 2022-03-28T12:06:31Z

Done. LMK if there's anything that needs more clarification.

esc · 2022-03-29T10:04:17Z

Done. LMK if there's anything that needs more clarification.

This looks better, thank you.

esc · 2022-03-29T10:04:24Z

/azp run

azure-pipelines · 2022-03-29T10:04:33Z

Azure Pipelines successfully started running 1 pipeline(s).

esc · 2022-03-29T12:58:17Z

llvmlite/binding/ffi.py

-try:
-    _lib_handle = importlib.resources.path(pkgname, _lib_name)
-    lib = ctypes.CDLL(str(_lib_handle.__enter__()))
-    # on windows file handles to the dll file remain open after


I'm not sure about this change yet. The comment seems to indicate that the context manager can not be used on Windows.

Isn't it ok to delete a file that has open file handles on Windows? This is definitely the way you would want this to behave on posix systems, as it's totally fine to keep a file descriptor open and unlink the file. This has the advantage that the filesystem is cleaned up.

I'm investigating this to check the windows behaviour.

@folded thank you for looking into this, if you can work out if or if not this comment is/was relevant and can explain it too, that would be most helpful!

esc · 2022-03-29T12:59:06Z

llvmlite/binding/ffi.py

+# 1) In a shared library available as a resource of the llvmlite package.
+#    This may involve unpacking the shared library from an archive.
+# 2) Linked directly into the main binary. Symbols may be resolved from the
+#    main binary by passing None as the argument to ctypes.CDLL.


Under what circumstances do we pass None?

None is the second value in the iteration, as produced when the context contextlib.nullcontext(None) is entered.

esc · 2022-03-29T12:59:49Z

llvmlite/binding/ffi.py

+        contextlib.nullcontext(None)):
+    try:
+        with lib_context as lib_path:
+            lib = ctypes.CDLL(lib_path and str(lib_path))


What is the intent of using and here. Is this something special about the type of `lib_context?

lib_path is either a pathlib.Path or None, lib_path and str(lib_path) evaluates to None if lib_path is None, or converts it to a string otherwise, which copes with the fact that ctypes.CDLL (on windows, at least) requires a string path, not a pathlib.Path

esc · 2022-03-29T13:00:59Z

@folded thank you again for the PR. CI seems green now and I have given the patch a more thorough review. There are a few questions left to address. Oh, and also: do you have any thoughts on how I could test this patch to verify it does indeed work in the kinds of environments you describe.

… to a temp file, and delete it after loading.

esc · 2022-06-01T13:18:31Z

@folded thank you again for submitting this. I will now continue to review this.

esc · 2022-06-01T13:21:09Z

llvmlite/binding/ffi.py

+            try:
+                with lib_context as lib_path:
+                    if os.name == 'nt' and not lib_path:
+                        # The Windows implementation of ctypes.CDLL does not


Question here: does this mean that Windows doesn't support the kind of static linking?

Unfortunately yes. It turns out that although Windows does have the ability to get a handle to the main binary, ctypes failed to implement that in a way that was consistent with MacOS/*nix. The only two solutions would be to send a PR to cpython to fix it or to use ctypes to dig into the win32 API and do it manually, neither of which I'm terribly keen on doing.

esc · 2022-06-01T13:27:00Z

@folded do you, by any chance, have some simple instructions for creating they type of binary that has llvmlite statically linked so that I can test that this works?

folded · 2022-06-01T13:28:06Z

I haven't had time to look at this for a while, but I think it might be sensible to split it into logically separate components. Dealing with all the separate issues that came up had quite a few follow-on effects.

Would that be helpful?

esc · 2022-06-01T13:30:14Z

I haven't had time to look at this for a while, but I think it might be sensible to split it into logically separate components. Dealing with all the separate issues that came up had quite a few follow-on effects.

Would that be helpful?

Yes, that sounds useful.

esc · 2022-08-11T17:44:50Z

@folded it's been a while since I last heard from you, how do you want to proceed with this one?

folded · 2022-08-22T10:08:33Z

My work for this week is to do the internal upgrade to 0.39.0, and as a result I'll be going through these patches again and cleaning them up. I'll get back to you soon.

esc · 2022-08-22T13:46:00Z

My work for this week is to do the internal upgrade to 0.39.0, and as a result I'll be going through these patches again and cleaning them up. I'll get back to you soon.

Thank you for the update!

folded · 2022-08-22T16:57:06Z

I've split this into 3 PRs, starting with folded:refactor-native-library-loading

folded added 3 commits March 25, 2022 12:22

shorten line

0471584

make ctypes.CDLL arg a string

1917313

Add comments to native code loading

6d7f1df

esc added the 4 - Waiting on author label Mar 29, 2022

esc reviewed Mar 29, 2022

View reviewed changes

folded added 9 commits April 9, 2022 13:39

Make symbol lookup lazy

9538cc0

use module getattr to make llvm_version_info lazy without changing API

073659c

Add a test that ensures that it is safe to extract the llvmlite dylib…

90c7d45

… to a temp file, and delete it after loading.

Make flake8 happy

7c0e395

Don't try to dlopen self on Windows.

7a31842

Fix imports

dbedff5

...

a5e43f5

Suppress cleanup errors on Windows

c9cff01

flake8 unhappy again

1356f0a

esc reviewed Jun 1, 2022

View reviewed changes

folded closed this Aug 22, 2022

This was referenced Aug 23, 2022

Make symbol resolution lazy #873

Open

Fix symbol resolution race #872

Open

Refactor native library loading #871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading llvmlite from the main executable. #829

Support loading llvmlite from the main executable. #829

folded commented Mar 25, 2022 •

edited

Loading

esc commented Mar 27, 2022

folded commented Mar 28, 2022

esc commented Mar 28, 2022

folded commented Mar 28, 2022

esc commented Mar 29, 2022

esc commented Mar 29, 2022

azure-pipelines bot commented Mar 29, 2022

esc Mar 29, 2022

folded Mar 30, 2022

esc Mar 30, 2022

esc Mar 29, 2022

folded Mar 29, 2022

esc Mar 29, 2022

folded Mar 29, 2022

esc commented Mar 29, 2022

esc commented Jun 1, 2022

esc Jun 1, 2022

folded Jun 1, 2022

esc commented Jun 1, 2022

folded commented Jun 1, 2022

esc commented Jun 1, 2022

esc commented Aug 11, 2022

folded commented Aug 22, 2022

esc commented Aug 22, 2022

folded commented Aug 22, 2022

Support loading llvmlite from the main executable. #829

Support loading llvmlite from the main executable. #829

Conversation

folded commented Mar 25, 2022 • edited Loading

esc commented Mar 27, 2022

folded commented Mar 28, 2022

esc commented Mar 28, 2022

folded commented Mar 28, 2022

esc commented Mar 29, 2022

esc commented Mar 29, 2022

azure-pipelines bot commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esc commented Mar 29, 2022

esc commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esc commented Jun 1, 2022

folded commented Jun 1, 2022

esc commented Jun 1, 2022

esc commented Aug 11, 2022

folded commented Aug 22, 2022

esc commented Aug 22, 2022

folded commented Aug 22, 2022

folded commented Mar 25, 2022 •

edited

Loading