Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

WIP: Break out features #89

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

wkatsak
Copy link
Contributor

@wkatsak wkatsak commented Dec 22, 2023

Hello,

I've created this PR to get comments on my strategy for this solution. If this is satisfactory, I'll clean everything up.

I'm working on a project targeting wasm in the browser (via wasm-pack / wasm-bindgen). For this use case, I obviously can't have any of the ZMQ code being included. I also don't particularly need the signing code (I am using cylinder instead).

The main idea is to keep the default behavior as it functioned previously, but also to allow bringing in the SDK using whatever subset of the features you need.

Thanks in advance for any feedback.

Sincerely,
Bill

@vaporos
Copy link
Contributor

vaporos commented Dec 22, 2023

Looks fine to me.

@vaporos
Copy link
Contributor

vaporos commented Dec 22, 2023

The best way to test this is going to be installing just and then running "just lint" and "just test". By modifying justfile, you can make it so that it tests any combination of features that you want to make sure work (i.e. if you want only one feature on, you could lint/test/build that way). The CI just runs "just build", "just lint", "just test".

@wkatsak
Copy link
Contributor Author

wkatsak commented Dec 22, 2023

The linter is not breaking because of anything I changed. The blocking error is as follows:

    Checking sawtooth-sdk v0.5.3 (/Volumes/Storage/taekion/sawtooth-sdk-rust)
error: unused `#[macro_use]` import
  --> src/lib.rs:18:1
   |
18 | #[macro_use]
   | ^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`


@vaporos
Copy link
Contributor

vaporos commented Jan 2, 2024

The error occurs because we lint with "cargo clippy --manifest-path=./Cargo.toml --no-default-features -- -D warnings" to make sure things aren't broken with all features off, and with your changes, all use of logging code is behind features.

Because logging only happens in the zmq code, you can just add this to fix it:

diff --git a/src/lib.rs b/src/lib.rs
index 6ee492d1..a865d868 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -15,6 +15,7 @@
  * ------------------------------------------------------------------------------
  */

+#[cfg(feature = "zmq")]
 #[macro_use]
 extern crate log;

Add feature-gating for the various sub-crates in the sdk. This
enables parts of the sdk (for example, the protos) to be used
in code that does not support zmq (for example, wasm targets).

Enable all features by default to avoid breaking any current users
of the sdk.

Signed-off-by: William Katsak <[email protected]>
@wkatsak
Copy link
Contributor Author

wkatsak commented Jan 9, 2024

I made the change suggested by @vaporos and added a better commit message. The tests look like they pass now.

@vaporos vaporos merged commit 0c586a4 into hyperledger-archives:main Jan 9, 2024
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants