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 Cargo features instead of cfg gating whole crates #3278

Closed
metajack opened this issue Sep 10, 2014 · 12 comments
Closed

Use Cargo features instead of cfg gating whole crates #3278

metajack opened this issue Sep 10, 2014 · 12 comments
Labels
A-infrastructure E-less-complex Straightforward. Recommended for a new contributor.

Comments

@metajack
Copy link
Contributor

Currently we add #[cfg(target_os=...)] lines to crates that are platform specific so they compile on every platform. We should instead use Cargo's new feature support.

@metajack metajack added the E-less-complex Straightforward. Recommended for a new contributor. label Sep 10, 2014
@gilles-leblanc
Copy link
Contributor

I see that there are #[cfg(target_os=...)] statements for extern crate definitions but also for different use and fn and other statements.

Is the scope of this issue only for the extern crates (like those present in lib.rs files)?

@metajack
Copy link
Contributor Author

Currently cargo must compile every dependency so even if a crate is conditionally included in the rust source, it must be declared as a dependency and compiled. Therefore for rust-core-foundation, we have to cfg out the whole crate at the top level. Most of the time, this is a few annotations, but in the case of glfw-rs, it was quite a bit.

Cargo features allow us to not compile crates on some platforms, so all those cfgs can be removed in rust-core-foundation and we just tell cargo not to include that feature unless we're on macos.

Does that help?

@gilles-leblanc
Copy link
Contributor

Yep, thanks. I'll start working on something and will probably push something to check if I'm on the right track.

@gilles-leblanc
Copy link
Contributor

The pull request for the features feature has just landed in Cargo.

@mbrubeck
Copy link
Contributor

I'm going to be reorganizing the code in components/compositing/platform, and will probably add feature gates to it in the process. Let me know (here or on IRC) if you've already made changes in that subdirectory, so we can avoid conflicts. I'll also link to this issue once I have a PR ready.

@gilles-leblanc
Copy link
Contributor

@mbrubeck I had originally said I was looking into it but then found out the "features" feature wasn't implemented in Cargo yet so I started working on something else in the mean time and didn't get back to it.

So I haven't done anything yet.

I'll unassign myself from this for the mean time.

@mylainos
Copy link

It look like features is implemented now.

@frewsxcv
Copy link
Contributor

This is possibly related to #5973

@faineance
Copy link
Contributor

Anyone mind if I pick this up?

@frewsxcv
Copy link
Contributor

Go for it!

@frewsxcv frewsxcv added the C-assigned There is someone working on resolving the issue label Dec 14, 2015
@wafflespeanut wafflespeanut removed the C-assigned There is someone working on resolving the issue label Jan 16, 2016
@schuster
Copy link
Contributor

A brief glance through some of the Cargo files suggests the problem was already fixed by making various dependencies target-specific. Is there anything left to do?

@mbrubeck
Copy link
Contributor

No, I think this is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infrastructure E-less-complex Straightforward. Recommended for a new contributor.
Projects
None yet
Development

No branches or pull requests

9 participants