Skip to content

Env override support #808

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Env override support #808

wants to merge 9 commits into from

Conversation

@lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Sep 17, 2019

See #804 The env var is RUST_STD_DETECT_UNSTABLE for now.

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

gnzlbg
gnzlbg approved these changes Sep 17, 2019
View reviewed changes
gnzlbg
gnzlbg reviewed Sep 17, 2019
View reviewed changes
gnzlbg
gnzlbg reviewed Sep 17, 2019
View reviewed changes
gnzlbg
gnzlbg suggested changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The only thing missing would be to add a test.

gnzlbg
gnzlbg reviewed Sep 17, 2019
View reviewed changes
@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 17, 2019

So this looks good. The only thing missing is a CI buildjob that uses the feature, e.g., on x86_64-unknown-linux-gnu, and then disables e.g. AVX and checks that AVX2 is enabled but AVX isn't.

The simplest thing I think is what you suggested, copying this buildjob (https://github.com/rust-lang/stdarch/blob/master/ci/azure.yml#L182) and just running the pertinent test.

@lu-zero
Copy link
Contributor Author

lu-zero commented Sep 17, 2019

I added the buildjob and shuffled around the style fixes.

gnzlbg
gnzlbg reviewed Sep 17, 2019
View reviewed changes
crates/std_detect/src/detect/mod.rs

impl Feature {
#[doc(hidden)]
pub fn from_str(_s: &str) -> Result<Feature, ()> { Err<()> }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax error

@bors
Copy link
Contributor

bors commented Sep 17, 2019

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

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 17, 2019

I think you merged this and now master is broken :D

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 17, 2019

Don't stress, keep calm. Just create a new branch from master, and submit a PR that fixes it :)

@gnzlbg gnzlbg closed this Sep 17, 2019
@lu-zero
Copy link
Contributor Author

lu-zero commented Sep 17, 2019

Ouch, when you ctrl^Rgit p+Enter on the wrong shell...

@lu-zero
Copy link
Contributor Author

lu-zero commented Sep 17, 2019

Hopefully I fixed the syntax error before the push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gnzlbg gnzlbg gnzlbg requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gnzlbg gnzlbg

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lu-zero @rust-highfive @gnzlbg @bors