Build without +nightly

[perlun]

First, thanks for a great library everybody! 😄

I happened to see this, and I tried the examples. They seem to run just fine on stable Rust nowadays. Any particular reason why we'd want to keep running these on nightly, or is just a remnant from the past?

[cuviper]

Are you sure you used stable for rayon-demo? There are a few uses of impl Trait, and the corresponding #![feature(conservative_impl_trait)] at the top of src/main.rs.

It probably wouldn't be too hard to move away from this though.

[nikomatsakis]

Removing the nightly dependency seems worthwhile.

[perlun]

Are you sure you used stable for rayon-demo

Ouch, you're right! I was seemingly running it with 1.17.0-nightly. 🤣

Anyway, let's keep this PR open until we have managed to remove the nightly dependency. @cuviper, can you point us in the right direction?

[nikomatsakis]

@perlun doing something like cargo +stable test --all ought to suffice (for uncovering errors, I mean)

[cuviper]

Well, cargo +stable test probably still won't work because of the #[bench] infrastructure. :(

But for plain cargo +stable build, we need to remove non-test uses of impl Trait. So at a minimum, the feature line here should be wrapped in #[cfg_attr(test, ...)], then try a build to let rustc tell you where that feature was being used. From a quick grep, looks like just tsp/graph.rs.

[nikomatsakis]

Well, cargo +stable test probably still won't work because of the #[bench] infrastructure. :(

Ah, yeah, forgot about that -- but you could do it the demo directories, I think, right?

We should write a stable version of compiletest. It's pretty silly for that to use unstable code.

[cuviper]

but you could do it the demo directories, I think, right?

I'm not sure what you mean. The demo directories are the only place we use #[bench] at all.

But generally speaking, yes, I'd like it better if most of our testing could run on stable too.

[nikomatsakis]

Oh, sorry, I was mixing things up. I thought you were referring to the compiletest stuff, which also requires nightly. Anyway.

[perlun]

@nikomatsakis I changed the PR to only fix the trivial cfg_attr thing @cuviper mentioned. For the rest I think I'll need some help; I'm unfortunately much less skilled in Rust at the moment than I'd wish for but I intend to improve it.

[cuviper]

@perlun if you'd like to join gitter, I can help with that.

[cuviper]

CI reveals that we don't actually do a plain build of rayon-demo, or the cfg_attr change would have broken it. So we should make sure to add that to .travis.yml too.

[perlun]

@cuviper Good point, fixed the Travis config to build the rayon-demo always now (i.e. for all rust versions in the build matrix).

[cuviper]

I think the test line needs to go back under nightly -- it will still need features for #[bench] -- but we should aim to do a plain cargo build for all platforms.

[bluss]

One can run benchmarks on stable with bencher, though I mostly don't do that because of the extra macro boilerplate. However, every day of more serious Rust usage brings more needs for a way to measure and understand performance with the stable compiler's codegen.

An up front disclaimer is that bencher is a stop-gap measure and not a project that tries to make a better bencher.

[perlun]

@cuviper @nikomatsakis It now builds on 3/4 configurations in the build matrix. Any suggestions on what we can do with the fourth? (it complained about "multiple versions of rayon" or something like that)

[cuviper]

Sorry, I think we've been talking past each other on this. I meant you should add:

   - cargo build --features="$FEATURES" -p rayon-demo

... leaving the matching cargo test under nightly only.

[cuviper]

@perlun I think changing the line as I suggested will solve the "multiple versions of rayon". compiletest-rs doesn't work well if there are multiple builds of librayon.rlib, which you get when you build with mismatched features in use. But there will still be some impl Iterator lines to solve for stable builds.

[perlun]

I think changing the line as I suggested will solve the "multiple versions of rayon". compiletest-rs doesn't work well if there are multiple builds of librayon.rlib, which you get when you build with mismatched features in use.

Ah, thanks for explaining.

I changed like you suggested but now get another error:

The command "cargo build --features="$FEATURES"" exited with 0.
$ cargo build --features="$FEATURES" -p rayon-demo
warning: unused manifest key: workspace.exclude
error: package id specification rayon-demo matched no packages

[cuviper]

Bah, the cargo with rust-1.12 doesn't handle -p rayon-demo properly through workspaces. Instead I think you can use --manifest-path=rayon-demo/Cargo.toml.

[perlun]

Bah, the cargo with rust-1.12 doesn't handle -p rayon-demo properly through workspaces. Instead I think you can use --manifest-path=rayon-demo/Cargo.toml.

😄 Thanks, I'll try.

Why do we need to support 1.12 still btw?

[cuviper]

It's a deliberate choice not to increase the minimum Rust required if we can help it. That matters less for rayon-demo though.

[perlun]

But there will still be some impl Iterator lines to solve for stable builds.

Yep, now we are down to them; that's what Travis is complaining about now.

[perlun]

@cuviper Bear with my ignorance, but what would be the proper way to implement these functions without using the rust-lang/rust#34511 unstable feature? Can it be done?

[cuviper]

Don't worry about asking questions!

At a first glance, try changing the return value to some nonsense, like -> (). You'll get tons of errors, but on the function in question you should be able to see what type rustc found.

So for Graph::all_nodes:

error[E0308]: mismatched types
  --> rayon-demo/src/tsp/graph.rs:28:9
   |
28 |         (0..self.num_nodes).map(Node::new)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found struct `std::iter::Map`
   |
   = note: expected type `()`
              found type `std::iter::Map<std::ops::Range<usize>, fn(usize) -> tsp::graph::Node {tsp::graph::Node::new}>`

We can't fully name that type ourselves, but the good news is that fn types coerce, so it should work with returning Map<Range<usize>, fn(usize) -> Node>.

Graph::edges is harder:

error[E0308]: mismatched types
  --> rayon-demo/src/tsp/graph.rs:51:9
   |
51 | /         self.all_nodes()
52 | |             .filter_map(move |target| {
53 | |                 self.edge_weight(source, target)
54 | |                     .map(|weight| {
...  |
60 | |                     })
61 | |             })
   | |______________^ expected (), found struct `std::iter::FilterMap`
   |
   = note: expected type `()`
              found type `std::iter::FilterMap<std::iter::Map<std::ops::Range<usize>, fn(usize) -> tsp::graph::Node>, [closure@rayon-demo/src/tsp/graph.rs:52:25: 61:14 self:_, source:_]>`

Not only is this building on top of the same type from the first example, but it also uses a closure which we literally cannot name at all. One possibility is to create a new custom Iterator type which holds the same data and references as the FilterMap and closure would.

The other way is to box it into a trait object, like Box<Iterator<Item=Edge> + 'a>, which has the downside of forcing dynamic dispatch, though it's possible the compiler might see through that. You can try it and see if the performance numbers change -- it might not matter.

[perlun]

@cuviper Thanks for the really detailed answer!

Unfortunately, I haven't yet found the time and energy to complete it. I don't work with Rust regularly so "switching mindset" to be able to productively get things done can be challenging for me...

If anyone else wants to pick this up and complete it, I won't mind. Otherwise, we're perhaps better off closing for now rather than letting it remain open for many months more.

[cuviper]

Ok, no problem. If someone wants to start a new PR, feel welcome!