Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented May 10, 2023

What does it do?

Currently the sc-client-db is pulled in with default features at the level cli which makes it a defacto dependency for all parachains. With the latest slew of compiler releases not include <cstdint> by default anymore, and rocksdb binding a significant amount of CI resources, I felt it was time to get rid of it without having to continually patch substrate.

What important points should reviewers know?

RocksDB is now optional two levels further up, behind another rocksdb feature flag defaulting to on. The default behavior should not have changed.

Is there something left for follow-up PRs?

node-testing makes some assumptions of having some identifiers around which are not upheld, it surfaced when testing no-default-features, but otherwise no change.


We are successfully using this with polkadot-v0.9.41 (plus a few patches) and clang-16 and gcc-13.

@niklasad1
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@niklasad1 niklasad1 added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. labels May 12, 2023
Copy link
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice, looks good

Co-authored-by: Niklas Adolfsson <[email protected]>
@drahnr
Copy link
Contributor Author

drahnr commented May 15, 2023

Are the cumulus and polkadot CI checks caused by the changeset or are they known issues?

Could I get another review :)

@niklasad1 niklasad1 requested a review from bkchr May 15, 2023 08:32
@niklasad1
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@niklasad1
Copy link
Contributor

Are the cumulus and polkadot CI checks caused by the changeset or are they known issues?

It's probably just because there were a bunch PRs merged recently that broke that, rebased the branch and it's seems to be okay but cumulus is currently broken because paritytech/cumulus#2574

So nothing to worry about

@bkchr
Copy link
Member

bkchr commented May 16, 2023

and rocksdb binding a significant amount of CI resources

ROCKSDB_LIB_DIR="${rocksdb}/lib"; solves this problem.

And why are you using node-cli in your parachain?

@drahnr
Copy link
Contributor Author

drahnr commented May 17, 2023

and rocksdb binding a significant amount of CI resources

ROCKSDB_LIB_DIR="${rocksdb}/lib"; solves this problem.

I am aware, which was my first quarrel with it. Now we had the gcc/clang update as part of our build container and again rocksdb made em all red. On the other handside, not all prebuilts can be linked with rocksdb due to missing symbols and friends. Time not worth spending, hence the radical option of cutting it out entirely.

And why are you using node-cli in your parachain?

We are not. sc-cli and sc-client are used, let me double check if the other changes are already part of master and were just missing in the 0.9.4x branch we use.

@drahnr drahnr closed this May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. T0-node This PR/Issue is related to the topic “node”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants