-
Notifications
You must be signed in to change notification settings - Fork 707
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
Make sp-keystore
no_std
-compatible and fix the build-runtimes-polkavm
CI job
#3363
Conversation
@@ -28,7 +27,7 @@ rand_chacha = "0.2.2" | |||
|
|||
[features] | |||
default = ["std"] | |||
std = ["codec/std", "sp-core/std", "sp-externalities/std"] | |||
std = ["codec/std", "dep:parking_lot", "sp-core/std", "sp-externalities/std"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std = ["codec/std", "dep:parking_lot", "sp-core/std", "sp-externalities/std"] | |
std = ["codec/std", "parking_lot", "sp-core/std", "sp-externalities/std"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkchr Hm... sure, I can do this, but just to confirm - do we want to never use the dep:
syntax unless necessary?
I'm asking because this is not just a stylistic choice. The nice thing about the new (well, new-ish) dep:
syntax it that it prevents an implicit feature from being defined, that is, if an optional
dependency is not referred to with a dep:
then cargo will implicitly define a new feature like this:
[features]
parking_lot = ["dep:parking_lot"]
And in consequence this now technically becomes part of the public API for the crate, so another crate which depends on the sp-keystore
can then say:
[dependencies]
sp-keystore = { version = "...", features = ["parking_lot"] }
...which doesn't make much sense because the parking_lot
dependency here is an internal dependency, so it makes zero sense for another crate to be able to enable it. And using it through dep:
prevents this, because then cargo
won't create this implicit feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay good points, then let's ignore what I said. I didn't realized that this would then not be exposed 👍
* master: (41 commits) Add Coretime to Westend (#3319) removed `pallet::getter` from `pallet-sudo` (#3370) gossip-support: add unittests for update authorities (#3258) [FRAME Core] remove unnecessary overrides while using derive_impl for frame_system (#3317) Update coretime-westend bootnodes (#3380) `im-online` removal cleanup: remove off-chain storage (#2290) Bump the known_good_semver group with 1 update (#3379) Fix documentation dead link (#3372) Make `sp-keystore` `no_std`-compatible and fix the `build-runtimes-polkavm` CI job (#3363) Remove unused `im-online` weights (#3373) Ensure referenda `TracksInfo` is sorted (#3325) rpc server: add rate limiting middleware (#3301) do not block finality for "disabled" disputes (#3358) fix(zombienet): docker `img` version to use in merge queues for bridges (#3337) Various nits and alignments for SP testnets found during bumping `polkadot-fellows` repo (#3359) Add broker pallet to `coretime-westend` (#3272) remove recursion limit (#3348) Update subkey README.md (#3355) Bump the known_good_semver group with 6 updates (#3347) Document LocalKeystore insert method (#3336) ...
…lkavm` CI job (paritytech#3363) Fixes paritytech#3352
Fixes #3352