Open lorbax opened 2 months ago
If they are unused shouldn't they be removed?
If they are unused shouldn't they be removed?
what should be removed?
so you get this among other warnings, is Variable
used anywhere in the code? if its used under a specific flag then those warning are fine, if its not used at all it should be removed..
warning: trait `Variable` is never used
--> v2/binary-sv2/no-serde-sv2/codec/src/codec/mod.rs:37:11
|
37 | pub trait Variable {
| ^^^^^^^^
|
= note: `#[warn(dead_code)]` on by default
so you get this among other warnings, is
Variable
used anywhere in the code? if its used under a specific flag then those warning are fine, if its not used at all it should be removed..warning: trait `Variable` is never used --> v2/binary-sv2/no-serde-sv2/codec/src/codec/mod.rs:37:11 | 37 | pub trait Variable { | ^^^^^^^^ | = note: `#[warn(dead_code)]` on by default
I get it using rustc 1.78, but I don't get it using rustc 1.75.
yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.
yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.
does this mean the default toolchain MSRV should be set to 1.75?
that means that it was, but we removed it cause it was creating issue with some text editors.
yep cause before 1.75 were enforced by the toml file. Now that is not anymore enforced otherwise we get incompatibility with some editors, we need to specify the version in the script that runs fmt and clippy. Would be also nice to specify it somewhere where new contributors can see it very well I don't want them to waste time in solving "future" clippy error.
does this mean the default toolchain MSRV should be set to 1.75?
The stratum project should work with any toolchain equal or above 1.75. All the warnings we are seeing should probably be addressed in a PR.
this will make CI more complicated. We will need to:
It will make also development experience worst, since you will need to run cargo clippy with whatever is rust last on CI.
But I'm not opposed to it. Both approaches are valid: (1) run everything with MSRV and (2) only compile with MSRV and run checks with last.
You are right that we should check MSRV, and it was added before removing the tool-chain pining https://github.com/stratum-mining/stratum/actions/workflows/rust-msrv.yaml. Currently it only checks build process, but I guess we should run everything(ie testing as well) with latest toolchain version and with MSRV.
I agree also that we should handle those warnings. currently it is kinda annoying. PRs are more than welcome (:
Another note, we are currently running Clippy
and Fmt
on Nightly
, unless we have specific features we need from Nightly
for those, I think we can run them on latest
toolchain, which I think most of devs are running usually(?)
Fmt must be runned on Nightly, not sure about clippy
@Fi3 why? I think you can run fmt
on any toolchain
cause we use a feature that work only on nightly I don't remeber whcih one but if you try to run it you will see
here you are:
user@user ~/s/s/protocols (dev)> cargo fmt
Warning: can't set `imports_indent = Block`, unstable features are only available in nightly channel.
Warning: can't set `imports_layout = Mixed`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
....
I have toolchain with rustc 1.78
If I compile
protocols
crate I get the following warningsPossibly also other crates in the stack are affected. I think that this arose after the removal of rust-version field from crates https://github.com/stratum-mining/stratum/pull/981
We should specify the toolchain every time that is called cargo build or cargo clippy
cargo +1.75 build
or set some sort of override, like setting theRUSTUP_TOOLCHAIN
env variable. This has to be done in every script (like in/scripts/clippy-on-all-workspaces.sh
or those that triggers MG tests, like this one. I am not practical with the issue, so perhaps there is a better solution.