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

Use cfg_attr to gate musl linking #700

Closed
wants to merge 4 commits into from
Closed

Conversation

Others
Copy link

@Others Others commented Jul 25, 2017

Fixes #684. Previously building libc for musl would fail, because we don't want to use #[link(...)] outside a stdbuild. This commit fixes that issue. (Although it may leave other targets broken, I'm not sure.)

Fixes rust-lang#684. Previously building libc for musl would fail, because we don't want to use #[link(...)] outside a stdbuild. This commit fixes that issue. (Although it may leave other targets broken, I'm not sure.)
@alexcrichton
Copy link
Member

Looks like CI is failing? Can you also include tests from #685?

@Others
Copy link
Author

Others commented Jul 26, 2017

@alexcrichton I'm already seeing more failures on travis, even with the style fix. I think this is mostly because we never want to issue a link when stdbuild is not enabled. How would you feel about gating all the link pragmas behind stdbuild? One way of doing that would be changing:

else if #[cfg(all(not(stdbuild), feature = "use_std"))] {

To:

else if #[cfg(any(not(stdbuild), feature = "use_std"))] {

@Others
Copy link
Author

Others commented Jul 26, 2017

Alternatively I can just introduce a fix for the rumprun target (which looks to be the only broken one), and leave the rest as-is.

@alexcrichton
Copy link
Member

Ah yeah it'd be great to add this same fix there as well!

@Others
Copy link
Author

Others commented Jul 27, 2017

@alexcrichton Okay, I've made the CI build work for all platforms. I did, however, use the first method I outlined and prevent any linking from happening if not(stdbuild). I think that the logic is cleaner this way--and could prevent future issues.

If you agree with the above assessment, then I think this is ready to be merged.

@alexcrichton
Copy link
Member

Hm yeah so the only gotcha there is that if you pull in the libc crate without the standard library you may get unresolved symbol errors. Is that the use case you have in mind or is libstd still pulled in elsewhere?

@Others
Copy link
Author

Others commented Jul 27, 2017

@alexcrichton Yeah, you're right. Fixing it this way won't work. But I don't think the earlier #[cfg_attr(...)] fix works either. (Since it prevents the necessary libc.a from being pulled in on musl if you aren't using the standard library.)

I'm not sure how to proceed here. Do you know of any other options we have?

@alexcrichton
Copy link
Member

It's true yeah, we could avoid the cfg inside the link attribute perhaps though? Basically have a #[cfg_attr(not(stdbuild), link(...))]?

@Others
Copy link
Author

Others commented Jul 27, 2017

@alexcrichton How can we avoid the cfg inside the link? Don't we want different behavior depending on whether target_feature = "crt-static"?

@alexcrichton
Copy link
Member

Yeah we wouldn't get precisely the same behavior, we'd have to select one kind of linakge for musl, for example.

@Others
Copy link
Author

Others commented Aug 2, 2017

@alexcrichton Do you have any thoughts on what we should do then? #684 is a serious problem for using Rust on a MUSL platform. If there is no quick fix for now, perhaps we should introduce some sort of work around?

@alexcrichton
Copy link
Member

I think our only option is to have #[link(name = "c")] and pray it works.

@clarfonthey
Copy link
Contributor

Is there any progress on this? I'd be willing to help out with a fix but I'm not sure where to go from here.

@Others
Copy link
Author

Others commented Aug 15, 2017

@clarcharr I'm still paying attention to this, although I've been busy at work this past week. I'm dissatisfied with the current solution / every solution outlined thus far. I feel like the #[link(name = "c")]approach is likely to break -crt-static linking (which I personally depend on). @alexcrichton do you have further thoughts on this? Do you want me to just forge ahead with the #[link(name = "c")] and pray strategy?

@alexcrichton
Copy link
Member

Ideally this would look like:

#[cfg_attr(all(not(stdbuild), target_feature = "crt-static"), link(name = "c", kind = "static")]
#[cfg_attr(all(not(stdbuild), not(target_feature = "crt-static")), link(name = "c")]

But right now target_feature = "crt-static" is unstable to match against. Short of that I'm not entirely sure what to do.

@bors
Copy link
Contributor

bors commented Aug 19, 2017

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

@alexcrichton
Copy link
Member

I'm going to close this due to inactivity, but feel free to resubmit with a rebase!

bors added a commit that referenced this pull request Oct 9, 2017
Attempt to fix musl build.

Follow-up to #685 and #700.
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.

Won't compile on musl without libstd
5 participants