llvmlite icon indicating copy to clipboard operation
llvmlite copied to clipboard

Support loading llvmlite from the main executable.

Open folded opened this issue 2 years ago • 13 comments

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.

folded avatar Mar 25 '22 12:03 folded

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.

esc avatar Mar 27 '22 15:03 esc

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.

folded avatar Mar 28 '22 09:03 folded

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.

esc avatar Mar 28 '22 10:03 esc

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

folded avatar Mar 28 '22 12:03 folded

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

This looks better, thank you.

esc avatar Mar 29 '22 10:03 esc

/azp run

esc avatar Mar 29 '22 10:03 esc

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Mar 29 '22 10:03 azure-pipelines[bot]

@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.

esc avatar Mar 29 '22 13:03 esc

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

esc avatar Jun 01 '22 13:06 esc

@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?

esc avatar Jun 01 '22 13:06 esc

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?

folded avatar Jun 01 '22 13:06 folded

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 avatar Jun 01 '22 13:06 esc

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

esc avatar Aug 11 '22 17:08 esc

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.

folded avatar Aug 22 '22 10:08 folded

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!

esc avatar Aug 22 '22 13:08 esc

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

folded avatar Aug 22 '22 16:08 folded