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

CLI - Replace clippy with a manual check #1928

Open
wants to merge 9 commits into
base: bfops/remove-io-macros
Choose a base branch
from

Conversation

bfops
Copy link
Collaborator

@bfops bfops commented Nov 1, 2024

Description of Changes

We were apparently only using clippy to check for nonfunctional print statements (#1819).

This PR replaces our use of clippy (when building modules) with a manual check for disallowed print statements.

For me, this reduced the time of cargo clean && time spacetime build from 2m15s (with clippy) to 1m30s (without clippy).

API and ABI breaking changes

Not breaking.

Expected complexity level and risk

2

Future work

The other items in #1819.

Testing

  • Manually added a println! to new module and observed that spacetime build produces an error:
$ spacetime build
info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date

Detected nonfunctional print statements:

./src/lib.rs:30:     println!("Hello world");

Error: Found 1 disallowed print statement(s).
These will not be printed from SpacetimeDB modules.
If you need to print something, use the `log` crate
and the `log::info!` macro instead.

crates/cli/src/tasks/rust.rs Outdated Show resolved Hide resolved
.run()?;
anyhow::ensure!(out.status.success(), "clippy found a lint error");
let mut err_count: u32 = 0;
for file in walkdir::WalkDir::new(project_path).into_iter() {
Copy link
Collaborator Author

@bfops bfops Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be walking the whole project directory? As-is, this erroneously catches some printlns in BitCraft's build.rs. Should we be hardcoding to only look in src/...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There does not appear to be a way to use e.g. cargo to list all the source files, as far as I can find. It looks like the best we could do is parse the output of the cargo build in order to figure out which files were involved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could add an extra argument that defaults to src/, e.g. --lint-path or something.

@bfops bfops marked this pull request as draft November 1, 2024 19:27
@bfops bfops mentioned this pull request Nov 1, 2024
3 tasks
@bfops bfops changed the base branch from master to bfops/remove-io-macros November 1, 2024 20:13
@bfops bfops marked this pull request as ready for review November 1, 2024 20:25
@bfops bfops added release-any To be landed in any release window CLI only This change only affects the CLI behavior labels Nov 4, 2024
@jdetter jdetter removed their request for review November 8, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI only This change only affects the CLI behavior release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant