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

Implement target-features flag #1556

Closed
wants to merge 1 commit into from

Conversation

srijs
Copy link

@srijs srijs commented Apr 29, 2015

This is a beginner's shot at #1137. Not sure this is the way you'd want to do this, but I'm happy to learn from mistakes ;)

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@alexcrichton
Copy link
Member

Thanks for the PR! Right now, however, Cargo isn't trying to expose an exhaustive set of flags from the compiler at the top level (that would be quite a few). Instead we've been holding out for extensible strategies to solve issues like #544 (for example #595).

It's also not 100% clear to me whether configuration like this belongs in the Cargo.toml's [profile] sections or on the command line.

@bors
Copy link
Contributor

bors commented Apr 29, 2015

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

@srijs
Copy link
Author

srijs commented Apr 29, 2015

Admittedly, I am scratching my own itch here, but I think it is important to expose that particular flag, because it is actually influencing whether certain non-portable builds succeed or not, while other arguments regarding "somehow passing flags" to the compiler have been revolving around adding flags to debug issues, or change subtleties in optimisation. The impression I had of the [profile] feature is that it serves the same purpose, specifying things like optimisation levels or debug information, which is why I think it might not be the right place for this flag.

@alexcrichton
Copy link
Member

The unfortunate part is that the compiler exposes many options like this, and Cargo either needs to bind all of them or provide a generic means of specifying them to the compiler. This is the purpose of the cargo rustc command partly (generically specifying flags for any crate).

Configuration options such as this are very target-specific, and also arguably belong in a custom target specification that's read in by the compiler.

@srijs
Copy link
Author

srijs commented May 1, 2015

Configuration options such as this are very target-specific, and also arguably belong in a custom target specification that's read in by the compiler.

Could you elaborate a bit on this? Is this a deliverable that would be independent from the "generic" solution cargo rustc?

@alexcrichton
Copy link
Member

Ah indeed! Custom target specifications are a means of passing a custom --target flag which is described by a JSON file. The file describes how all artifacts for that target should be built, including the set of features which are passed down to LLVM.

@alexcrichton
Copy link
Member

Closing due to may last comment, plus cargo rustc is now implemented!

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.

4 participants