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

feat: remove unused dependencies #5144

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Jan 27, 2023

Description

Removed all unused dependencies.
Includes [package.metadata.cargo-machete] for false positives

How Has This Been Tested?

Manual, unit tests

Fixes: #5136

@@ -42,5 +39,5 @@ wasm-bindgen-test = "0.3.28"

[features]
avx2 = ["tari_crypto/simd_backend"]
js = ["getrandom/js", "js-sys"]
Copy link
Member

Choose a reason for hiding this comment

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

Might be needed to provide random to JS. Guessing machete might not take this into account

Copy link
Collaborator

Choose a reason for hiding this comment

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

The wasm tests should fail if this is the case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I manually tested this with 'cargo build - - all-features' if the wasm build with this then it is it should be fine. I picked up 2 issues with this

Copy link
Contributor

Choose a reason for hiding this comment

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

cargo test isn't even compiling on this branch.

anyway, the js feature is needed by the wasm feature right beneath it.

test locally in base_layer/key_manager/ by running make test

https://github.com/tari-project/tari/blob/development/base_layer/key_manager/Makefile#L4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Js feature is not removed, its still there. Like you say its used by the wasm feature branch.
This PR only removes "getrandom/js"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at the CI, the key manager wasm tests all pass.

@SWvheerden
Copy link
Collaborator Author

Its weird that the cargo test compiles and runs fine locally not on CI
Means we have some dependency that is not properly defined

@CjS77
Copy link
Collaborator

CjS77 commented Jan 30, 2023

I'm happy
👍

@SWvheerden
Copy link
Collaborator Author

It all green now,
Just added in the last ignore for machete.
This PR will still complain about openssl in the wallet_ff cargo. But it will get removed here: #5137

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Happy to merge this PR, but I think it's worth adding a few CI jobs in this PR as well.

  1. Add a cargo machete job
  2. Confirm the wasm build is being done

@CjS77
Copy link
Collaborator

CjS77 commented Jan 31, 2023

1 can def be a separate PR
2 can be a separate PR too, even if the wasm build is broken tbh

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM

@CjS77 CjS77 merged commit a9d0f37 into tari-project:development Jan 31, 2023
@SWvheerden SWvheerden deleted the sw_remove_unused_deps branch February 2, 2023 09:06
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.

Clean up cargo toml files
5 participants