Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider removing C dependency for popular platforms #61

Closed
matklad opened this issue Apr 1, 2020 · 4 comments · Fixed by #63
Closed

Consider removing C dependency for popular platforms #61

matklad opened this issue Apr 1, 2020 · 4 comments · Fixed by #63

Comments

@matklad
Copy link

matklad commented Apr 1, 2020

From reading #32, my understanding is that this C code is only really needed on some obscure platforms. Notably, modern windows, linux and mac all use thread safe dlerrors, and hence don't need global_static.c (but I have very clouded understanding of the original problem here, so I might be talking nonsense here).

It would be cool if that C code wasn't needed in such cases, by either detecting the target platform in build.rs, or by providing an explicit feature flag for the user.

The reason why I care about absence of C deps is that in rust-analyzer we've implemented a blanket check that the build does not call a C compiler. This is not a super-strong requirement, but it's nice to, eg, know that you can do cross compilation without futzing with C compilers.

@nagisa
Copy link
Owner

nagisa commented Apr 1, 2020

My own memory of the issue is fairly dated now, so there may be errors in it, but the keyword here is modern. The reason I ended up unconditionally compiling this snippet for any unix is because I do not really want to spend time looking into which version of glibc introduced MT-safe dlerror, and then trying to figure out if those versions are at all relevant to anyone, anymore. And that’s the work I’d have to do just to stop doing this for glibc targets. There are also musl, uclibc, macos (to which I don’t have direct access) etc.

As per this comment the relevant standards only began allowing (allowing here is the keyword) MT-safe dlerror in 2008. While that was 12 years ago at this point, it is very plausible that there are still environments which not only take advantage of "allow" in the new versions of the POSIX standard, but actually adhere to the "must be program-wide global" from the older versions of it.

If we made use of this snippet conditional, I fear that this code-path would end up bit-rotting to the point of not working anymore on the less used platforms.

Overall I’d prefer stabilization of the weak linkage in Rust so that this could be implemented without C and without sacrificing anything else, But if C is a problem I’m open to try alternative approaches too.

@matklad
Copy link
Author

matklad commented Apr 2, 2020

so that this could be implemented without C and without sacrificing anything else

I think using weak linkage here would be a perfect work-around, rather than a perfect solution. Like, on those platform where dlerror is MT-safe the perfect solution is to just not do anything on top. I however totally understand the perspective that, if work-around is not tested in the common case, it won't work in the edge case.

But if C is a problem I’m open to try alternative approaches too.

It is a minor problem, we can either remove our deny_c hack, or just use a fork of libloading (rust-analyzer is a binary, so we don't need to worry about several versions of libloading at all).

Still, my understanding here is that the problem this tries to solve is a niche of a niche (several version of libloading on an old platform), the solution is very elaborate, and that it's not 100% full-proof either (as the user can call dlerror/dlopen directly themselves, bypassing the check). I definitely don't have full context here, but my gut feeling that just documenting

Note that on some old platforms dl* family of functions is not thread-safe, and libloading might mysteriously break if you call those functions direcly, or if some other library does that. Note that some other library might include an earlier version of libloading.

might be a good enough solution. This can further be strengthened by yanking old major versions of libloading.

@nagisa
Copy link
Owner

nagisa commented Apr 2, 2020

Yeah, documenting the caveat is probably a good alternative too.

@nagisa
Copy link
Owner

nagisa commented Apr 5, 2020

@matklad Released 0.6.0. Since removing the dependency involved a minor, but breaking nevertheless, change to the Library::get behaviour, I had to make a minor (rather than patch) version bump. I ended up making a number of other breaking changes while I was at it, so porting to 0.6.0 may not be as trivial as just changing the version number, but hopefully not too extremely difficult. Changelog is here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants