Integrate dylint lints to build
#1412
Conversation
dylint-based ink! linting rules become mandatory, because we can check ink! compilation errors with them (see use-ink/ink#1976)
| fn lint( | ||
| dylint: bool, | ||
| extra_lints: bool, |
There was a problem hiding this comment.
I don't like that all the lints will be executed on each build, because it slows down the build process and may bother the user with extra noise generated by lints. Ideally, we should execute only mandatory lints needed to check compilation errors (like no_main) and skip the rest.
I think the most simple way to do this is to split our ink_linting in two dylint libraries, like ink_linting and ink_linting_extra. The first one will contain only the mandatory lints, and the second one will run the optional analyses when user requesting it.
There are other ways to suppress lints (like hacking Cargo.toml in contract projects or compilation flags for rustc), but AFAIK these will execute lints anyway, suppressing their output.
WDYT @ascjones?
There was a problem hiding this comment.
Yes we only want the mandatory lints to execute every time, extra lints must still be opt-in.
I think the most simple way to do this is to split our ink_linting in two dylint libraries, like ink_linting and ink_linting_extra
Sounds good to me 👍
There was a problem hiding this comment.
Merged use-ink/ink#2032 now
| fn lint( | ||
| dylint: bool, | ||
| extra_lints: bool, |
There was a problem hiding this comment.
Yes we only want the mandatory lints to execute every time, extra lints must still be opt-in.
I think the most simple way to do this is to split our ink_linting in two dylint libraries, like ink_linting and ink_linting_extra
Sounds good to me 👍
Split `ink_linting` to `ink_linting_mandatory` and `ink_linting`. Mandatory lints will be integrated in the `cargo-build` build process in use-ink/cargo-contract#1412. Extra lints are optional and could be run by the contract developer to highlight possible issues with secure coding style and to check the compliance with best practices. For more information about this design decision, see: use-ink/cargo-contract#1412 (comment). Closes use-ink#2006
ink_linting was splitted up to two libraries in use-ink/ink#2032
This is necessary to expand `cfg_attr` macros in the contract as for
on-chain build. Particulary, we need this to enforce the `no_std`
attribute on the crate level, because `Cargo.toml` usually contains
`features` entry for local builds:
```
[features]
default = ["std"]
std = [
"ink/std",
]
```
|
It is ready for review; however, we need use-ink/ink#2032 to be merged first. This will allow us to install the |
* feat(linter): Split `ink_linting` into two libraries Split `ink_linting` to `ink_linting_mandatory` and `ink_linting`. Mandatory lints will be integrated in the `cargo-build` build process in use-ink/cargo-contract#1412. Extra lints are optional and could be run by the contract developer to highlight possible issues with secure coding style and to check the compliance with best practices. For more information about this design decision, see: use-ink/cargo-contract#1412 (comment). Closes #2006 * chore: Update CHANGELOG * feat: Run linting tests in github-actions * fix(ci): Add caching for `linting` builds * fix(ci): gh-actions syntax * fix(ci) * fix(ci): Remove `--locked` flag for linting This is necessary, because the lockfile needs to be updated since we run build with a different `rust-toolchain.yml`.
|
use-ink/ink#2060 will enable us to fix the CI issues here. |
Reuse the previous approach with hacking the workspace manifest in the contract project. We cannot add those libraries as direct dependencies to cargo-contract because we use a different version of the toolchain. To make it possible for these libraries to be found at runtime, we would also have to hack dylint.
Otherwise, it takes the toolchain version from `rust-toolchain.toml`
This is necessary to avoid installing two versions of toolchains.
|
It passed the CI and could be merged. But I would like to draw attention to a few points:
|
|
While I agree with idea of making it simple, I'm not sure about For I am beginning to wonder whether we should start to encourage using a docker workflow, especially for workshops because getting setup with |
|
I think we should avoid requiring a particular Rust toolchain for |
There is a test that checks if `cargo-dylint` is not installed for the native toolchain. Right now we can left it as it is. It will be fixed when we automate installation of the toolchain.
|
I have removed the fixed toolchain version. Instead, I provide users with warnings and instructions on how to install the required toolchain. This occurs only if the user compiles for the RiscV target or explicitly runs the build with the I suggest not adding any complicated logic to toolchain management in the next release. This approach will give us time to discuss it and ensure that nothing is broken. |
Summary
inkorpallet-contracts?Description
This PR follows up use-ink/ink#2001 making
dylint-based lints mandatory in the build process. This change allows us to introduce ink-specific compilation errors more flexibly through the dylint-based ink_linting module.Checklist before requesting a review
CHANGELOG.md