NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

unstable rust features in code? #158

Closed felixhekhorn closed 2 years ago

felixhekhorn commented 2 years ago

In https://github.com/N3PDF/pineappl/commit/f4f47f63e7118eb43479be8b3eec8fa5b53fc1fa I changed some string outputs - and so I had to also adjust the tests, which I hope I did - but I cannot test it because I get

$ cargo test
   Compiling pineappl v0.5.4 (/home/felix/Physik/N3PDF/PineAPPL/pineappl/pineappl)
error[E0658]: trait bounds other than `Sized` on const fn parameters are unstable
   --> pineappl/src/sparse_array3.rs:244:6
    |
244 | impl<T: Clone + Default + PartialEq> SparseArray3<T> {
    |      ^
...
322 |     pub const fn dimensions(&self) -> (usize, usize, usize) {
    |     ------------------------------------------------------- function declared as const here
    |
    = note: see issue #93706 <https://github.com/rust-lang/rust/issues/93706> for more information

For more information about this error, try `rustc --explain E0658`.
error: could not compile `pineappl` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

the mentioned command says something that I have to activate nightly to use unstable features - which is not what I want and moreover PineAPPL is supposed to work with 1.56.1

just for reference

$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/felix/.rustup

stable-x86_64-unknown-linux-gnu (default)
rustc 1.60.0 (7737e0b5c 2022-04-04)

however, I was able to install the CLI (whose installation instructions I updated as well by the way)

alecandido commented 2 years ago

I guess it has been solved in Rust 1.61.0:

const fn signatures can now use impl Trait in argument and return position

And since now Rust 1.62 is stable, I guess there is no problem.

felixhekhorn commented 2 years ago

no problem - as long as we change https://github.com/N3PDF/pineappl/blob/f4f47f63e7118eb43479be8b3eec8fa5b53fc1fa/docs/installation.md#L122

alecandido commented 2 years ago

no problem - as long as we change

Perfectly fine by me, I'd simply replace 1.56.1 by stable, and we'll always make sure it is going to work on stable. What do you think @cschwan?

cschwan commented 2 years ago

I'd like to avoid changing the MSRV since I know a few people like to use Rust from their Linux distribution. Commit 6c2b1aca7bb5e2dc391aae4eafd04a5fa53a29de should fix the problem. Please re-open if that isn't the case!

felixhekhorn commented 2 years ago

cargo test passed :+1: (after I remembered to run cargo build first - it is quite strange test is not running it on it's own ... )

cschwan commented 2 years ago

cargo test can take quite long to finish which is why it doesn't run by default.