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

Add sonames on musl #268

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Add sonames on musl #268

merged 1 commit into from
Jul 25, 2022

Conversation

nekopsykose
Copy link
Contributor

the musl platform also supports shared objects having an SONAME, and
this function is called only when making a 'shared object', so this
should always be added

fixes 04a92bc where this was removed
even when generating a dynamic library forcefully by setting library
type to cdylib

fixes #267

@nekopsykose
Copy link
Contributor Author

nekopsykose commented Jul 22, 2022

reverting all of 04a92bc instead does give warnings with the default -unknown-linux-musl toolchains:

warning: dropping unsupported crate type `cdylib` for target `x86_64-unknown-linux-musl`

(though they are of course harmless)

but when staticlib is used, this code path is entirely unused (it's called shared_object_link_args and only gets called when making a cdylib), so when someone does have a musl toolchain that can make cdylibs, the soname should be there; this conditional was always broken (we are making a shared library but skipping only the soname and nothing else, which doesn't make sense to do)

@nekopsykose
Copy link
Contributor Author

nekopsykose commented Jul 22, 2022

if there was a way to detect 'target supports cdylib' we could fix #180 too, but i don't think there is a way (without just eating the warnings), so leaving that is fine

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2022

Codecov Report

Merging #268 (4b9bc37) into master (41f5562) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   82.01%   81.99%   -0.02%     
==========================================
  Files          12       12              
  Lines        1846     1844       -2     
==========================================
- Hits         1514     1512       -2     
  Misses        332      332              
Impacted Files Coverage Δ
src/target.rs 88.88% <100.00%> (-0.16%) ⬇️
src/build.rs 81.56% <0.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@lu-zero
Copy link
Owner

lu-zero commented Jul 24, 2022

Please run rustfmt on your commits :)

the musl platform also supports shared objects having an SONAME, and
this function is called only when making a 'shared object', so this
should always be added

fixes 04a92bc where this was removed
even when generating a dynamic library forcefully by setting library
type to cdylib

fixes #267
@lu-zero lu-zero merged commit 38942b5 into lu-zero:master Jul 25, 2022
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 this pull request may close these issues.

built shared objects with --library-type cdylib do not have an SONAME on musl
3 participants