Allow static and dynamic linking #1620

jsgf · 2019-09-16T07:07:34Z

Currently bindgen always uses clang-sys with the "runtime" feature -
that is, load libclang at runtime with dlopen (or similar)
at runtime. This PR keeps this default, but also

adds "static" to statically link libclang
without either "runtime" or "static", link with the shared library

Many distributions don't ship with a static libclang, but linking with the dynamic
library will use normal ld.so mechanisms to define where the .so file should be found.
(Ditto for the Mac and Windows equivalents.)

Fixes #1617

bors-servo · 2019-09-17T00:36:29Z

☔ The latest upstream changes (presumably 3062841) made this pull request unmergeable. Please resolve the merge conflicts.

emilio

Huh, I thought ::is_loaded() and co would not work on statically-linked stuff.

This looks great, do you know if it'd be a lot of work to at least cargo-check the new build configuration? That'd prevent it from breaking in the future.

jsgf · 2019-09-17T01:15:46Z

Huh, I thought ::is_loaded() and co would not work on statically-linked stuff.

It doesn't - they're all behind #[cfg(feature = "runtime")].

This looks great, do you know if it'd be a lot of work to at least cargo-check the new build configuration? That'd prevent it from breaking in the future.

I'll see what I can do.

emilio · 2019-09-17T01:25:47Z

Then how does this build?

jsgf · 2019-09-17T18:17:16Z

Then how does this build?

? I added the necessary #[cfg(...)] conditional build checks to lib.rs.

emilio · 2019-09-18T00:55:53Z

Sorry, we were talking past each other looks like :)

I was talking about the individual function's is_loaded() checks, like clang_Foo::is_loaded() that we have sprinkled around in the codebase.

But looks like clang-sys stubs them properly: https://github.com/KyleMayes/clang-sys/blob/ad5b1d0c64b19c4bfe00df6dfd661b80203dd161/src/link.rs#L195

jsgf · 2019-09-19T22:01:42Z

@emilio The checks are failing because this doesn't support libclang <= 4.0 any more - it tries to resolve symbols which don't exist in those versions. It might work with "runtime" because it only dynamically looks up symbols which do exist.

emilio · 2019-09-20T03:15:34Z

@emilio The checks are failing because this doesn't support libclang <= 4.0 any more - it tries to resolve symbols which don't exist in those versions. It might work with "runtime" because it only dynamically looks up symbols which do exist.

Yeah, that's ~expected, I guess... Can we not test building the static version in those rather than removing the platforms? What's what triggers the static feature?

Alternatively, we need to remove a bunch of tests/expectations/libclang-3.* and same for 4... I can do that as a followup but I'd like to keep at least 4.0 if we can (clang 3 is pretty old anyway at this point).

jsgf · 2019-09-20T16:34:16Z

It's triggered by non-"runtime". (nothing) is link with whatever is available (static or dynamic), and "static" is specifically statically link. "static" is hard to test because libclang doesn't build static out of the box and I haven't found any prebuilt libclangs available as static. But clang-sys does support static, so presumably someone is using it (well, including me, but we're using a slightly different mechanism).

Are you suggesting:

delete the 3.x tests
make the 4.x tests conditional on "runtime"

?

bors-servo · 2019-10-07T23:40:39Z

☔ The latest upstream changes (presumably 807fa1e) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2019-10-07T23:42:11Z

(FWIW, I haven't forgotten about this and still looks good. Will try to figure out a way to test this properly without regressing the dynamic clang support while I address similar CI issues due to the clang 9.0 release, probably over the weekend or on friday)

Thanks again for this!

emilio · 2019-10-07T23:42:34Z

(And really sorry for the lag replying, I had missed your previous comment)

jsgf · 2019-10-09T14:38:24Z

Do you need me to rebase it?

emilio · 2019-10-10T00:45:45Z

That'd be ideal, but no worries if you don't get to it.

…

On 10/9/19 4:38 PM, Jeremy Fitzhardinge wrote: Do you need me to rebase it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1620?email_source=notifications&email_token=AAKDBOV6FOG55UNUWVP23C3QNXUGRA5CNFSM4IW6Q6C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAYDR5Y#issuecomment-540031223>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKDBOVA4WDPOAGVCLWLGIDQNXUGRANCNFSM4IW6Q6CQ>.

emilio · 2019-10-11T16:38:16Z

#1646 is the version that adds support for libclang 9 on CI. That should allow to test this, and we should be able to avoid testing on other platforms.

We get link-time missing symbols with llvm before 5.x, so don't try to directly link with them.

Currently bindgen always uses clang-sys with the "runtime" feature - that is, load libclang at runtime with dlopen (or similar) at runtime. This PR keeps this default, but also - adds "static" to statically link libclang - without either "runtime" or "static", link with the shared library Many distributions don't ship with a static libclang, but linking with the dynamic library will use normal ld.so mechanisms to define where the .so file should be found. (Ditto for the Mac and Windows equivalents.)

jsgf · 2019-10-11T18:50:12Z

Rebased; fixed trivial conflict

emilio · 2019-10-18T13:49:25Z

So I put in #1649 my current WIP.

One annoyance of having two separate features is that downstream crates that want to use no-default-features can't get the advantages of the runtime feature, or otherwise they need to specify it, which means that other crates can't specify static.

I think ideally we'd make runtime built into clang-sys, but not sure how easy to do that is...

jsgf · 2019-10-18T21:02:20Z

I think that's a fundamental problem with using features this way - it isn't intended to support non-cumulative build-time options, and trying to do so breaks things. On the other hand, there's no better suited alternative mechanism.

emilio · 2019-10-19T03:29:15Z

So I gave a shot at finishing this, but apparently there are no pre-built static libclang versions, so we'd need to build our own which is not great...

Is the plan to build your own libclang.a and distribute it statically linked? Anyhow I guess I can land the patch without the tests if you need it and got it working.

jsgf · 2019-10-21T16:32:47Z

Yes, the clang build system seems to make generating a static libclang really tricky, and I couldn't find any Linux distro which ships a prebuilt static libclang. However our internal libclang has a static build, so its useful for that.

The non-static, non-runtime bindgen build will link to the shared libclang at build time which is logically identical to static linking from a code perspective - it just has the deployment question of whether the libclang is available at runtime at a path where it can be found.

So I'm not too concerned about not specifically testing static so long as non-runtime also works. This isn't ideal but 🤷‍♂.

bors-servo · 2019-10-21T21:06:54Z

☔ The latest upstream changes (presumably c462b63) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2019-10-21T21:07:12Z

Ok, so I landed #1649 with your changes and a slightly different/smaller set of CI tweaks, so that this is tested to the extent possible based on the comment above.

Thanks for the patch!

lopopolo · 2020-02-11T03:22:02Z

Hi @emilio this fix added a new default feature which wasn't mentioned in the changelog. This made upgrading my dependency with default-features = false tricky because I couldn't track down the errors from clang-sys.

Can we add a note to the changelog when features are added?

emilio · 2020-02-11T12:53:04Z

Yeah, that's fair, I added a changelog note just now, but I'll keep it in mind for the future too. Thanks :)

As it became non-default in rust-lang/rust-bindgen#1620.

* Fix bindings. PR #74 introduced a manually-generated change to the bindings that I accidentally broke in #76. * Specify the "runtime" feature of bindgen. As it became non-default in rust-lang/rust-bindgen#1620. * Make binding generation work properly on Windows too.

highfive added the S-awaiting-review label Sep 16, 2019

jsgf force-pushed the link-clang branch from 8655f8d to cc98574 Compare September 16, 2019 07:08

highfive added the S-needs-rebase label Sep 17, 2019

emilio approved these changes Sep 17, 2019

View reviewed changes

jsgf force-pushed the link-clang branch from cc98574 to b46e643 Compare September 17, 2019 18:25

jsgf force-pushed the link-clang branch from b46e643 to 0f2a98c Compare September 19, 2019 22:03

jsgf force-pushed the link-clang branch from 0f2a98c to 5638e2b Compare September 20, 2019 16:54

jsgf and others added 2 commits October 11, 2019 11:47

Drop support for llvm 3.x, and only support 4.x with "runtime"

d92cdf7

We get link-time missing symbols with llvm before 5.x, so don't try to directly link with them.

jsgf force-pushed the link-clang branch from 5638e2b to f5bb354 Compare October 11, 2019 18:49

emilio closed this Oct 21, 2019

emilio mentioned this pull request Oct 28, 2019

Allow statically linking libclang #1617

Closed

emilio mentioned this pull request Mar 10, 2020

Bump bindgen. RustAudio/coreaudio-sys#34

Merged

emilio added a commit to emilio/lmdb-rs that referenced this pull request Mar 10, 2020

Specify the "runtime" feature of bindgen.

0eaa4e9

As it became non-default in rust-lang/rust-bindgen#1620.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow static and dynamic linking #1620

Allow static and dynamic linking #1620

jsgf commented Sep 16, 2019

bors-servo commented Sep 17, 2019

emilio left a comment

jsgf commented Sep 17, 2019 •

edited

Loading

emilio commented Sep 17, 2019

jsgf commented Sep 17, 2019 •

edited

Loading

emilio commented Sep 18, 2019

jsgf commented Sep 19, 2019

emilio commented Sep 20, 2019

jsgf commented Sep 20, 2019 •

edited

Loading

bors-servo commented Oct 7, 2019

emilio commented Oct 7, 2019

emilio commented Oct 7, 2019

jsgf commented Oct 9, 2019

emilio commented Oct 10, 2019 via email

emilio commented Oct 11, 2019

jsgf commented Oct 11, 2019

emilio commented Oct 18, 2019

jsgf commented Oct 18, 2019

emilio commented Oct 19, 2019

jsgf commented Oct 21, 2019

bors-servo commented Oct 21, 2019

emilio commented Oct 21, 2019

lopopolo commented Feb 11, 2020

emilio commented Feb 11, 2020

Allow static and dynamic linking #1620

Allow static and dynamic linking #1620

Conversation

jsgf commented Sep 16, 2019

bors-servo commented Sep 17, 2019

emilio left a comment

Choose a reason for hiding this comment

jsgf commented Sep 17, 2019 • edited Loading

emilio commented Sep 17, 2019

jsgf commented Sep 17, 2019 • edited Loading

emilio commented Sep 18, 2019

jsgf commented Sep 19, 2019

emilio commented Sep 20, 2019

jsgf commented Sep 20, 2019 • edited Loading

bors-servo commented Oct 7, 2019

emilio commented Oct 7, 2019

emilio commented Oct 7, 2019

jsgf commented Oct 9, 2019

emilio commented Oct 10, 2019 via email

emilio commented Oct 11, 2019

jsgf commented Oct 11, 2019

emilio commented Oct 18, 2019

jsgf commented Oct 18, 2019

emilio commented Oct 19, 2019

jsgf commented Oct 21, 2019

bors-servo commented Oct 21, 2019

emilio commented Oct 21, 2019

lopopolo commented Feb 11, 2020

emilio commented Feb 11, 2020

jsgf commented Sep 17, 2019 •

edited

Loading

jsgf commented Sep 17, 2019 •

edited

Loading

jsgf commented Sep 20, 2019 •

edited

Loading