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

Fix build error with i128_support on Rust 1.26 #459

Closed
wants to merge 1 commit into from

Conversation

hcpl
Copy link

@hcpl hcpl commented May 16, 2018

This PR leaves i128 support opt-in -- I don't know if it should be automatically enabled for Rust 1.26+.

Also let me know if you prefer to manually parse output from rustc --version to keep rand with fewer dependencies.

Fixes #458.

@dhardy
Copy link
Member

dhardy commented May 16, 2018

Interesting; hadn't thought about solving this issue this way. The main concern I have is that there is no guarantee that either RUSTC is set or that rustc is in the path; better not to panic if this fails (just assume old Rustc, possibly with a warning).

@hcpl
Copy link
Author

hcpl commented May 16, 2018

The main concern I have is that there is no guarantee that either RUSTC is set or that rustc is in the path; better not to panic if this fails (just assume old Rustc, possibly with a warning).

That makes sense. I think this can be worked out as

extern crate version_check;

use std::io::{self, Write};

fn main() {
    match version_check::is_min_version("1.26.0") {
        Some((true, _version)) => {
            println!("cargo:rustc-cfg=stable_i128");
        },
        Some(_) => (),
        None => {
            // Not `eprintln!` because it is only available from Rust 1.19.
            writeln!(io::stdout(), "couldn't query version info from rustc").unwrap();
        },
    }
}

@hcpl
Copy link
Author

hcpl commented May 16, 2018

I just found out that this same fix can be backported to rand 0.3 as well if it is still supported.

@dhardy
Copy link
Member

dhardy commented May 16, 2018

I don't see much need to update 0.3 (are there still any users?) but it would be good to apply this to master as well (since we still compilers back to 1.22).

@pitdicker
Copy link
Contributor

On master this should already work with the latest stable, but only with the i128_support feature, not automatic. Otherwise it is a bug.

I have a few things I am not sure about:

  • We try to keep Rand light on dependencies. We don't depend on byteorder or lazy_static, while that would be helpful. And that is for core functionality. Do we want to add a dependency for what is only a little convenience?
  • Do we want to do another release of the 0.4 series? And one that is not purely a bug fix, but that in a way enables a new feature?
  • Can using the build.rs trick in any way cause problems for crates depending on Rand?

@dhardy
Copy link
Member

dhardy commented May 16, 2018

I checked the dependency and it is quite small, though as you say still an extra dependency.

Eventually the dependency will go away anyway (once we update the minimum supported version).

The disadvantage of making it opt-in is that we need to support the i128_support feature even after it can be enabled by default to avoid breaking builds. But perhaps making the support transparent is worse (because builds of apps using i128 on old compilers will fail for obscure reasons).

@pitdicker
Copy link
Contributor

Eventually the dependency will go away anyway (once we update the minimum supported version).

Yes, as soon as 1.26 is the minimum version this will solve itself. In a way I like being able to depend on the Rust version, but want to make sure we don't do it casually.

The disadvantage of making it opt-in is that we need to support the i128_support feature even after it can be enabled by default to avoid breaking builds.

I am not sure what you mean. Invalid/unrecognized features are just ignored.

@dhardy
Copy link
Member

dhardy commented May 16, 2018

Are they?

$ cargo test --features foo
error: Package `rand v0.5.0-pre.2 (file:///home/dhardy/rust/rand)` does not have these features: `foo`

So we need to keep old features in the Cargo.toml, though don't need to do anything in code.

@dhardy
Copy link
Member

dhardy commented May 16, 2018

So nothing to do for master then.

Do we want to make a new 0.4 release for this? I don't see much point since should have 0.5 out very soon and this is only to enable 128-bit types on stable compilers, though it's not a big deal if we must.

@dhardy
Copy link
Member

dhardy commented May 17, 2018

I'm going to close this then. Master (soon to be 0.5) already supports i128_support on stable compilers. If @hcpl or anyone else still wants stable i128 support on the 0.4 branch, they should open a new PR based on how the master branch achieves this:

#![cfg_attr(all(feature="i128_support", feature="nightly"), allow(stable_features))] // stable since 2018-03-27
#![cfg_attr(all(feature="i128_support", feature="nightly"), feature(i128_type, i128))]

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.

3 participants