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

Add MSRV check to make_release.sh #159

Closed
cschwan opened this issue Jul 21, 2022 · 13 comments
Closed

Add MSRV check to make_release.sh #159

cschwan opened this issue Jul 21, 2022 · 13 comments
Assignees

Comments

@cschwan
Copy link
Contributor

cschwan commented Jul 21, 2022

To avoid instance like #158 we need to add a check in make_release.sh that checks for cargo msrv being available and then performs the MSRV check.

@cschwan cschwan self-assigned this Jul 21, 2022
@alecandido
Copy link
Member

alecandido commented Jul 21, 2022

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

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

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

@cschwan
Copy link
Contributor Author

cschwan commented Jul 22, 2022

@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
Copy link
Contributor

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
Copy link
Contributor Author

cschwan commented Jul 22, 2022

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

alecandido commented Jul 22, 2022

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
Copy link
Contributor Author

cschwan commented Jul 22, 2022

@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
Copy link
Member

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

@cschwan
Copy link
Contributor Author

cschwan commented Jul 22, 2022

Implemented in commit ef91388.

@cschwan
Copy link
Contributor Author

cschwan commented Jul 22, 2022

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

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
Copy link
Contributor Author

cschwan commented Aug 19, 2022

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

@cschwan cschwan closed this as completed Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants