define MSRV and check for it in CI #1098
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -22,6 +22,12 @@ Netavark is a tool for configuring networking for Linux containers. Its features | |||
| - [Podman](https://podman.io/docs) 4.0+ | |||
| - [protoc](https://grpc.io/docs/protoc-installation/) | |||
|
|
|||
| ## MSRV (Minimum Supported Rust Version) | |||
|
|
|||
There was a problem hiding this comment.
Add a sentence here: "Netavark is guaranteed to build on this Rust version and later." or similar
There was a problem hiding this comment.
Given rust weird stance on backwards compatibility I find guaranteed to be a very strong word, i.e. it is not so recent that I think rust 1.80 failed to compile older version of the time crate
rust-lang/rust#127343 (comment)
i.e. it is impossible to build aardvark-dns 1.10.0 with rust v1.80 or newer. To their credit they provide a helpful error but still
error[E0282]: type annotations needed for `Box<_>`
--> /home/pholzing/.cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.30/src/format_description/parse/mod.rs:83:9
|
83 | let items = format_items
| ^^^^^
...
86 | Ok(items.into())
| ---- type must be known at this point
|
= note: this is an inference error on crate `time` caused by an API change in Rust 1.80.0; update `time` to version `>=0.3.35` by calling `cargo update`
And given we to not test all rust versions there might be some weird stuff in between so I think I rather s/guaranteed/supposed in your sentence
There was a problem hiding this comment.
"We test that Netavark will build on the following Rust version. Newer versions should also build, and if they do not, the issue should be reported and will be fixed. Older versions are not guaranteed to build and issues will not be fixed"?
| DOCKERFILE="$(dirname "${BASH_SOURCE[0]}")/Dockerfile.Rust" | ||
|
|
||
| # get the tag from the Dockerfile so we do not duplicate it | ||
| TAG=$(awk -F':' '/FROM/{print $NF}' $DOCKERFILE) |
There was a problem hiding this comment.
What would you think of if [[ -z "$TAG" ]]; then echo "FOO! empty tag"; exit 1; fi or equivalent?
Define the MSRV as 1.76 and document it the README. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Add a new image based of the offical rust image so we can expliclty check for the MSRV and not depend on distro version where we never really know what version we are getting. We cannot use the offical image directly as we need protobuf to be installed so create a new Dockerfile and add a script to push it to quay.io/libpod. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Ensure we can build our MSRV and delete the old ubuntu/centos task instead that was often way to outdated. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
We no longer test these here so remove them to avoid any confusion. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
LGTM |
|
this is awesome |
|
/lgtm |
Like done in netavark, containers/netavark#1098. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
...and you even remembered to update the descriptions on quay, for this and the other two images. Thank you. |
Define 1.76 as our MSRV currently and add a new container image that we use in CI to test that.