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

Use #![deny(unused_crate_dependencies)] instead of udeps #391

Merged
merged 8 commits into from Mar 16, 2023

Conversation

xgreenx
Copy link
Contributor

@xgreenx xgreenx commented Mar 16, 2023

The CI fails with the latest nightly Rust for udeps. This PR removes the usage of the udeps and replaces it with unused_crate_dependencies. Because of specific how linting works, I moved all tests folders into the src folder and compile integration tests as one module. Also, it improved the compilation time for tests(see for details why). Also found several unused dependencies with unused_crate_dependencies

@xgreenx xgreenx self-assigned this Mar 16, 2023
@xgreenx xgreenx changed the title Use #![deny(unused_crate_dependencies)] instead of unused Use #![deny(unused_crate_dependencies)] instead of udeps Mar 16, 2023
@xgreenx xgreenx requested a review from a team March 16, 2023 18:35
# Note that, while we don't use this dependency directly, we must specify it in
# order to enable the transitive dependency's "js" feature which is required
# for supporting the wasm32-unknown-unknown target.
[target.'cfg(target_arch = "wasm32")'.dependencies]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm only trying to figure out this part. @mitchmindtree Added it before=) He may know if we still need it or not. Usually, the js feature is required if you try to use the std code in the WASM.

cargo check --target wasm32-unknown-unknown -p fuel-crypto --no-default-features compiles successfully for me

Copy link
Member

Choose a reason for hiding this comment

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

I think this was potentially to help address the wasm issues the indexer was having? Does it still compile to wasm with --features random enabled?

Copy link
Member

Choose a reason for hiding this comment

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

hmm it seems we can't build fuel-crypto for wasm with secp256k1 enabled 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it still compile to wasm with --features random enabled?

Yeah, it compiles with cargo build --target wasm32-unknown-unknown -p fuel-crypto --no-default-features --features random

@xgreenx xgreenx marked this pull request as ready for review March 16, 2023 19:00
@bvrooman
Copy link
Contributor

If you're trying to address the ICE, it looks like it was already fixed by the Rust team: rust-lang/rust#109199. We might just need to wait until the next nightly version for CI to work again.

But finding the unused dependencies is a nice bonus here.

@xgreenx
Copy link
Contributor Author

xgreenx commented Mar 16, 2023

Another bonus of all tests in one module is that it compiles tests for me much faster. I would say 10 seconds versus 1 min=)

@Voxelot
Copy link
Member

Voxelot commented Mar 16, 2023

Another bonus of all tests in one module is that it compiles tests for me much faster. I would say 10 seconds versus 1 min=)

Makes sense, that's why we use a custom test harness in fuel-core so we don't have to link rocksdb into dozens of binary targets for each integ test module.

[dev-dependencies]
bincode = { version = "1.3", default-features = false }
criterion = "0.3"
fuel-crypto = { path = ".", default-features = false, features = ["random"] }
fuel-crypto = { path = ".", default-features = false, features = ["random", "serde"] }
Copy link
Member

Choose a reason for hiding this comment

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

should this be only enabled via the serde feature flag?

Copy link
Contributor Author

@xgreenx xgreenx Mar 16, 2023

Choose a reason for hiding this comment

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

Removed it, but it adds code like

#[cfg(feature = "serde")]
mod serde;
#[cfg(not(feature = "serde"))]
use bincode as _;

fuel-crypto = { workspace = true, default-features = false, features = ["random"] }
fuel-tx = { path = ".", features = ["builder", "random"] }
fuel-tx = { path = ".", features = ["builder", "random", "serde"] }
Copy link
Member

Choose a reason for hiding this comment

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

Also should only be enabled when the serde feature is turned on

Suggested change
fuel-tx = { path = ".", features = ["builder", "random", "serde"] }
fuel-tx = { path = ".", features = ["builder", "random"] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

#[cfg(feature = "serde")]
mod bytes;
#[cfg(feature = "serde")]
mod display;
#[cfg(not(feature = "serde"))]
use bincode as _;

@xgreenx xgreenx added this pull request to the merge queue Mar 16, 2023
Merged via the queue into master with commit 8ecd91c Mar 16, 2023
25 checks passed
@xgreenx xgreenx deleted the feature/remove-unused-ci branch March 16, 2023 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants