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.

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

Support stable rustc #107

Merged
merged 3 commits into from
Apr 9, 2018
Merged

Support stable rustc #107

merged 3 commits into from
Apr 9, 2018

Conversation

messense
Copy link
Contributor

No description provided.

laumann
laumann reviewed Mar 15, 2018
View reviewed changes
Copy link
Collaborator

@laumann laumann left a comment

Choose a reason for hiding this comment

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

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?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
src/runtest.rs Outdated Show resolved Hide resolved
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}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@laumann
Copy link
Collaborator

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 messense changed the title Proof of concept: support stable rustc Support stable rustc Mar 27, 2018
@messense
Copy link
Contributor Author

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.

@phansch phansch mentioned this pull request Apr 7, 2018
Closed
@oli-obk
Copy link
Contributor

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
View reviewed changes
Copy link
Collaborator

@laumann laumann left a comment

Choose a reason for hiding this comment

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

@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?

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

[features]
stable = ["compiletest_rs/stable"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, this is how to reference features from dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reference here: https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the pointer!

src/runtest.rs
tail.rotate_left(data.len());
// FIXME: Remove this when rotate_left is stable in 1.26
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! This is much more clear, thank you.

@laumann
Copy link
Collaborator

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
Copy link
Collaborator

laumann commented Apr 9, 2018

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

@laumann laumann merged commit 6fd4d0b into Manishearth:master Apr 9, 2018
@messense messense deleted the stable branch April 9, 2018 08:08
@laumann
Copy link
Collaborator

laumann commented Apr 9, 2018 •
edited
Loading

0.3.10 is out!

@iliekturtles iliekturtles mentioned this pull request Apr 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laumann laumann laumann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@messense @laumann @oli-obk