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

Add MSRV check to `make_release.sh` #159

Closed cschwan closed 1 year ago

cschwan commented 2 years ago

To avoid instance like https://github.com/N3PDF/pineappl/issues/158 we need to add a check in make_release.sh that checks for cargo msrv being available and then performs the MSRV check.

alecandido commented 2 years ago

Then, I'd start running this in a workflow. Possibly in the release workflow indeed (i.e. first makes checks, and if everything's fine, finally release - otherwise just crash).

The workflow can then be triggered manually. Or on release (not tags), that is just another button in GitHub.

alecandido commented 2 years ago

Moreover, cargo msrv is rather popular (at least it seems), but it's still community maintained. Since we are always updating to newer Rust, we'll completely rely on it. So, occasional checks should still be run manually...

felixhekhorn commented 2 years ago

Just to say: (as far as I understood) by pinning a version you're breaking one of the features of rust: rolling release ...

cschwan commented 2 years ago

@felixhekhorn we'd only check that we don't increase the minimum supported version of Rust (MSRV) for released versions. That does the opposite of breaking things: we make sure that people with older Rust installations can still install PineAPPL. But maybe I misunderstood you?

felixhekhorn commented 2 years ago

I was more thinking on a higher meta level: pinning a version is a valid choice of software design (that's why poetry is there in the first place), but (as far as I understand) this is not the same philosophy in the rust universe: here the idea is to release a new version every 6 weeks and the development continuously adds new features ... - I guess that's why @AleCandido wanted to point to stable (whatever current stable is) instead of a concrete number

cschwan commented 2 years ago

But we're not pinning anything, we simply set the minimum version from which on you can use any higher version! In any case I'd like avoid forcing people to always update their Rust compilers.

alecandido commented 2 years ago

Indeed, I agree, in principle keep supporting up to a fixed version is not breaking rolling release.

The only bad effect is that you accumulate extra burden for the developers: you are increasing the number of versions you have to support. So, if ever they will fix or add something you useful, you can not use out of the box, unless you bump MSRV.

cschwan commented 2 years ago

@AleCandido true, and at some point we can increase the MSRV again. But for the time being 1.56 must suffice. If you wonder why exactly that version: it's the first version to support the 2021 edition and the rust-version field in Cargo.toml.

alecandido commented 2 years ago

That I knew, I found out while I was debugging the readthedocs configs :)

cschwan commented 2 years ago

Implemented in commit ef9138817de99fa026dcb617ada46c5129bf477f.

cschwan commented 2 years ago

Then, I'd start running this in a workflow. Possibly in the release workflow indeed (i.e. first makes checks, and if everything's fine, finally release - otherwise just crash).

The workflow can then be triggered manually. Or on release (not tags), that is just another button in GitHub.

Doing this in a workflow would be ideal, but I'm not sure whether that's possible. At the very least we'd need to pass a version string to the workflow.

alecandido commented 2 years ago

This we can do. We can also store the MSRV in a file, or read from: https://github.com/N3PDF/pineappl/blob/ef9138817de99fa026dcb617ada46c5129bf477f/pineappl/Cargo.toml#L12

for best consistency.

cschwan commented 1 year ago

I'm closing this Issue, and further discussion can happen in #168.