core::crypto: features cleanup in tests
#1983
Conversation
core::crypto: features cleanup in tests
substrate/primitives/core/Cargo.toml
Outdated
| @@ -132,6 +132,7 @@ serde = [ | |||
| "secrecy/alloc", | |||
| "sp-core-hashing", | |||
| "sp-storage/serde", | |||
| "sp-runtime-interface/disable_target_static_assertions", | |||
There was a problem hiding this comment.
This probably should not be here. However without this compilation fails:
error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
--> substrate/primitives/runtime-interface/src/impls.rs:45:1
|
45 | assert_eq_size!(usize, u32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: source type: `usize` (64 bits)
= note: target type: `u32` (32 bits)
= note: this error originates in the macro `assert_eq_size` (in Nightly builds, run with -Z macro-backtrace for more info)
There was a problem hiding this comment.
Yeah this is wrong and needs to be removed!
There was a problem hiding this comment.
Substrate currently assumed that no_std == arch::wasm32. Shitty, I know, but that is currently the assumption.
There was a problem hiding this comment.
Yes, ideally that assert should be changed to check for wasm32 and not for no_std, and then you won't need to disable it.
...as Basti said, the current assumption everywhere is that no_std equals WASM, which, uh, I guess I'll have to fix soon-ish since I'll be wanting to get the RISC-V based runtimes working. :P
There was a problem hiding this comment.
Shall we remove this one too?
|
Is serde still relevant? |
At the moment: yes. We had some discussion on removing this feature and inclusion of |
| @@ -569,6 +577,7 @@ mod test { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[cfg(feature = "std")] | |||
There was a problem hiding this comment.
As we only run tests in std why do we need all this feature gating?
There was a problem hiding this comment.
Yes. I want e.g. time cargo test --release -p sp-core --no-default-features --features="serde" to be executed correctly.
There was a problem hiding this comment.
But you don't run this anymore? And serde is mainly about testing that it compiles?
There was a problem hiding this comment.
I want to run it locally. I want to have ability to test the sp-core with the different feature sets it offers. serde was just an example, full-crypto or full-crypto,serde are the other options.
There was a problem hiding this comment.
We have trait methods definition under std, so w/o this paricular gate you will get:
cargo +nightly check --tests --no-default-features --features="full_crypto"
error[E0599]: no function or associated item named `from_full` found for struct `ecdsa::Public` in the current scope
--> substrate/primitives/core/src/ecdsa.rs:588:12
|
80 | pub struct Public(pub [u8; PUBLIC_KEY_SERIALIZED_SIZE]);
| ----------------- function or associated item `from_full` not found for this struct
...
588 | Public::from_full(
| ^^^^^^^^^ function or associated item not found in `Public`
For more information about this error, try `rustc --explain E0599`.
error: could not compile `sp-core` (lib test) due to previous error
There was a problem hiding this comment.
I want to run it locally. I want to have ability to test the
sp-corewith the different feature sets it offers.serdewas just an example,full-cryptoorfull-crypto,serdeare the other options.
These features are not changing anything in the implementation, they only unlock different kind of functions etc. When you run everything using std you test the logic. We also don't have logic in there that is different based on features. So, not sure what you want to test there.
There was a problem hiding this comment.
We have trait methods definition under
std, so w/o this paricular gate you will get:
cargo +nightly check --tests --no-default-features --features="full_crypto"
Because you are using --tests. Just drop this, these tests only need to work on std.
There was a problem hiding this comment.
IMO the tests should be runnable for any set of features provided by this module. The point of this PR was to check if tests are fine for particular features (or combinations of them).
If we always want to test with default features, then this PR does not make sense.
There was a problem hiding this comment.
I mean checking that it compiles make sense. (without the tests). To ensure that these features compile on their own.
* removed FromBridgedChainMessageDispatch in favor of XcmBlobMessageDispatch * use HaulBlobExporter/HaulBlobExporterAdapter instead of XcmBridge/XcmBridgeAdapter * tests for sending/dispatching messages * use new schema in testnet bridges + some cleanup * clippy * spelling + added TODO * cleanup some checks * benchmarks compilation * all is XCM * updated README.md * ref issue from TODO
There are three features that affect how
cryptomodule is compiled:std,full_cryptoandserde.Following invocations are now failing:
This PR adds the features to the test, so these commands are working properly.
Some groundwork for: #1984