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

imp: in fixed-hash allow opting out rand when std enabled #804

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

Farhad-Shabani
Copy link
Contributor

@Farhad-Shabani Farhad-Shabani commented Dec 1, 2023

Closes: #805

fixed-hash/Cargo.toml Outdated Show resolved Hide resolved
@bkchr bkchr requested a review from ordian December 1, 2023 11:04
@ordian ordian added breaking-change Breaking change changelog Needs to be added to the changelog labels Dec 1, 2023
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

This is a breaking change unfortunately.

We'd also need to bump MSRV to 1.60 here according to https://doc.rust-lang.org/cargo/reference/features.html#dependency-features:

Note: The ? syntax is only available starting with Rust 1.60.

We don't have an automatic check for MSRV currently..

@dzmitry-lahoda
Copy link
Contributor

@ordian what is next if you know?

think is that this thing (along with others) hampers adoption of non browser std wasm usage of these things. and before that was annoying in usage in pure parity codebase (that js compilation issue)

@ordian
Copy link
Member

ordian commented Dec 5, 2023

@ordian what is next if you know?

think is that this thing (along with others) hampers adoption of non browser std wasm usage of these things. and before that was annoying in usage in pure parity codebase (that js compilation issue)

Tests are failing (exactly because this is a breaking change). They need to be fixed, then it can be merged and later released as a breaking change.

But I am not sure why you are blocked on this. I don't see any std gated code in this crate. Could you just disable std feature and add explicitly crate dependencies you need std features from (e.g. byteorder) with std feature enabled as a workaround?

@dzmitry-lahoda
Copy link
Contributor

When I use this crate with std it makes wasm32 std project fail to compile. If use in wasm32 std but not setting std only on this crates it works. But breaks right away as dep of dep because this crate is used a lot, and sets honestly srt on this crate.
So I am sure cannot use this crate without std because need to make that in all deps, ibc rs, ether abi gen, parity xcm. Impossible.

@ordian
Copy link
Member

ordian commented Dec 5, 2023

Yeah, sometimes you can't control which features are selected due to transitive dependencies and cargo's feature unification.

@dzmitry-lahoda
Copy link
Contributor

@Farhad-Shabani

seems tests are failing

   Compiling bounded-collections v0.1.9 (/Users/runner/work/parity-common/parity-common/bounded-collections)
error[E0599]: no function or associated item named `random` found for struct `H256` in the current scope
Error:   --> kvdb-rocksdb/examples/memtest.rs:51:25
   |
51 |         Self::with_seed(H256::random())
   |                               ^^^^^^ function or associated item not found in `H256`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `kvdb-rocksdb` (example "memtest") due to previous error
Error: warning: build failed, waiting for other jobs to finish...
Error: The process '/Users/runner/.cargo/bin/cargo' failed with exit code 101

@ordian ordian merged commit 8723455 into paritytech:master Dec 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaking change changelog Needs to be added to the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fixed-hash: allow opting out rand when std enabled
4 participants