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

Support stable rustc #107

Merged
merged 3 commits into from Apr 9, 2018

Conversation

Projects
None yet
3 participants
@messense
Copy link
Contributor

messense commented Mar 13, 2018

No description provided.

@laumann
Copy link
Owner

laumann left a comment

I like the idea! Being able to run on stable is quite nice, and the implementation is nice and "opt-in", just how I like it, so thanks :-)

Please look at my comments, I think they should be manageable and then we could merge this.

src/lib.rs Outdated
@@ -272,6 +275,8 @@ pub fn make_test_closure(config: &Config, testpaths: &TestPaths) -> test::TestFn
let config = config.clone();
let testpaths = testpaths.clone();
test::DynTestFn(Box::new(move || {
#[cfg(feature = "norustc")]
let config = config.clone(); // FIXME: why is this needed?

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

Maybe some of the NLL changes in rustc nightly obviates the need to clone config? Or we're looking at a bug in rustc nightly? config has type &Config, so it should be freely copyable across threads, right? It could also be a that's been fixed in nightly, but hasn't made its way to stable?

This comment has been minimized.

@messense

messense Mar 15, 2018

Author Contributor

It could be related to test::DynTestFn takes an Box<FnMut() + Send> instead of Box<FnBox() + Send>. (Because FnBox isn't stable)

Ref: servo/rustc-test@aa605ed

src/json.rs Outdated
@@ -24,7 +24,6 @@ struct Diagnostic {
level: String,
spans: Vec<DiagnosticSpan>,
children: Vec<Diagnostic>,
rendered: Option<String>,

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

This just fixes the build warning, right? Could I have that in a separate PR?

This comment has been minimized.

@messense

messense Mar 15, 2018

Author Contributor

Sure.

This comment has been minimized.

@messense

messense Mar 15, 2018

Author Contributor

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

Thanks


// FIXME: Remove this when rotate_left is stable in 1.26
#[cfg(feature = "norustc")]
fn read2_abbreviated(mut child: Child) -> io::Result<Output> {

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

This looks quite different from the standard version of read2_abbreviated(), why is that?

This comment has been minimized.

@messense

messense Mar 15, 2018

Author Contributor

This is just because I don't want to copy the slice::rotate_left impl for now(It should be done if we are going to merge this), see the FIXME above.

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

I was just wondering why read2_abbreviated() was affected by the norustc feature, when it's because of rotate_left. I'd rather have an:

// FIXME: Remove this when rotate_left is stable in 1.26
#[cfg(feature = "norustc")]
fn rotate_left(..) -> {
    ...
}

to switch implementations of rotate_left.

Cargo.toml Outdated
@@ -23,6 +23,7 @@ tempdir = { version = "0.3", optional = true }
serde = "1.0"
serde_json = "1.0"
serde_derive = "1.0"
rustc-test = { git = "https://github.com/messense/rustc-test.git", branch = "update", optional = true}

This comment has been minimized.

@laumann

laumann Mar 15, 2018

Owner

What's in this crate? Could you add it to crates.io?

This comment has been minimized.

@messense

messense Mar 15, 2018

Author Contributor

It's a fork of servo/rustc-test with updates from rustc. It should be upstreamed to servo/rustc-test.

This comment has been minimized.

@messense

messense Mar 24, 2018

Author Contributor

I have opened a PR in rustc-test here: servo/rustc-test#8

@laumann

This comment has been minimized.

Copy link
Owner

laumann commented Mar 15, 2018

How does the norustc feature relate to this? Does norustc now imply works-on-stable? It could be a separate feature stable that implies norustc, couldn't it?

messense added some commits Mar 13, 2018

@messense messense changed the title Proof of concept: support stable rustc Support stable rustc Mar 27, 2018

@messense

This comment has been minimized.

Copy link
Contributor Author

messense commented Mar 27, 2018

To move this forward, I have released the updated version of rustc-test which works on stable to crates.io under name tester since servo/rustc-test seems not actively maintained.

@oli-obk

This comment has been minimized.

Copy link

oli-obk commented Apr 7, 2018

I don't have enough words to express my happiness about this PR!

@laumann

laumann approved these changes Apr 8, 2018

Copy link
Owner

laumann left a comment

@oli-obk I should not be so slow then :-)

@messense Sorry for the absence, had a week off. I think this looks good to go.

@SergioBenitez Any comments on this?

@@ -8,3 +8,6 @@ authors = ["Thomas Jespersen <laumann.thomas@gmail.com>"]
[dev-dependencies.compiletest_rs]
path = ".."
features = ["tmp"]

[features]
stable = ["compiletest_rs/stable"]

This comment has been minimized.

@laumann

laumann Apr 8, 2018

Owner

Oh, this is how to reference features from dependencies?

This comment has been minimized.

@messense

This comment has been minimized.

@laumann

laumann Apr 8, 2018

Owner

Thanks for the pointer!

tail.rotate_left(data.len());
// FIXME: Remove this when rotate_left is stable in 1.26

This comment has been minimized.

@laumann

laumann Apr 8, 2018

Owner

Nice! This is much more clear, thank you.

@laumann

This comment has been minimized.

Copy link
Owner

laumann commented Apr 9, 2018

Hmm, I checked this out and did:

$ cd test-project
$ cargo +stable test
error[E0554]: #![feature] may not be used on the stable release channel
  --> /home/tj/rust/compiletest-rs/src/lib.rs:13:39
   |
13 | #![cfg_attr(not(feature = "norustc"), feature(rustc_private))]
   |                                       ^^^^^^^^^^^^^^^^^^^^^^^

error[E0554]: #![feature] may not be used on the stable release channel
  --> /home/tj/rust/compiletest-rs/src/lib.rs:14:38
   |
14 | #![cfg_attr(not(feature = "stable"), feature(test))]
   |                                      ^^^^^^^^^^^^^^

error[E0554]: #![feature] may not be used on the stable release channel
  --> /home/tj/rust/compiletest-rs/src/lib.rs:15:38
   |
15 | #![cfg_attr(not(feature = "stable"), feature(slice_rotate))]
   |                                      ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

error: Could not compile `compiletest_rs`.

To learn more, run the command again with --verbose.

Am I doing something wrong? I'm on rustc 1.25.0

@laumann

This comment has been minimized.

Copy link
Owner

laumann commented Apr 9, 2018

Of course, I have to do cargo +stable --features stable :P

@laumann laumann merged commit 6fd4d0b into laumann:master Apr 9, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@messense messense deleted the messense:stable branch Apr 9, 2018

@laumann

This comment has been minimized.

Copy link
Owner

laumann commented Apr 9, 2018

0.3.10 is out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.