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

Merged
merged 25 commits into from
Jan 17, 2025
Merged

CLI - Replace clippy with a manual check #1928

merged 25 commits into from
Jan 17, 2025

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:
$ spacetimedb-lcli 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.
  • BitCraft fails by default
BitCraft/packages/game$ spacetimedb-cli build

Detected nonfunctional print statements:

./src/game/world_gen/world_generator.rs:53: //         //println!("{} {}", x, y);

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.
  • BitCraft passes once the println! is removed:
BitCraft/packages/game$ sed -i'' 's/println!/log::info!/g' src/game/world_gen/world_generator.rs
BitCraft/packages/game$ spacetimedb-cli build
    Updating crates.io index
  Downloaded spacetimedb v1.0.0-rc3
  Downloaded 1 crate (26.7 KB) in 0.25s
   Compiling thiserror v1.0.58
   Compiling log v0.4.21
   Compiling shlex v1.3.0
   Compiling itertools v0.12.1
   Compiling cc v1.2.9
...
  • BitCraft fails if I forcibly pass a different lint-dir that includes the println!s in build.rs:
BitCraft/packages/game$ spacetimedb-cli build --lint-dir .

Detected nonfunctional print statements:

././src/game/world_gen/world_generator.rs:53: //         //println!("{} {}", x, y);
././build.rs:45:     println!("cargo:rerun-if-changed={dir}");
././build.rs:98:     println!("cargo:rerun-if-changed=src/messages/components.rs");
././build.rs:492:         // output_commit.push_str(format!("            println!(\"{}: {{:?}}\", knowledge.entries);\n", n).as_str());

Error: Found 4 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.
  • --lint-dir '' skips linting:
$ spacetimedb-cli build --lint-dir ''
Warning: Skipping checks for nonfunctional print statements.
If you have used builtin macros for printing, such as println!,
your logs will not show up.
   Compiling bitcraft-spacetimedb v0.1.0 (/mnt/nutera/work/BitCraft/packages/game)

@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
@bfops bfops changed the base branch from bfops/remove-io-macros to master December 4, 2024 18:00
@bfops bfops force-pushed the bfops/replace-clippy branch from 9d84c16 to dcf325b Compare December 4, 2024 18:03
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed testing spec. It's a little unfortunate that we warn even in commented code. I didn't think about that, but also that's not a very common case (although in principle it could come up). I'm good to approve this. We can consider an even better solution post 1.0

@bfops bfops added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 16, 2025
@bfops bfops added this pull request to the merge queue Jan 17, 2025
Merged via the queue into master with commit a54ea3a Jan 17, 2025
9 checks passed
@bfops bfops deleted the bfops/replace-clippy branch January 17, 2025 17:49
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.

2 participants