Lift dependencies to workspace and move api-client to its own package #848

clangenb · 2025-05-01T08:36:39Z

The biggest motivation for this check is that encointer needs to depend on crates-io versions of the polkadot-sdk, and it is just so much easier for us to patch upstream if all the deps are defined in the workspace root only. Additionally, this gives you also small benefits like

ensuring that we use the same crate version throughout the whole workspace
update deps at one place only

Follow-ups:

This PR unveils that some of our CI stuff has only been running for the main crate substrate-api-client instead of all the crates because the root toml was both, a package toml and a workspace toml. As this also implies fixing of some clippy warnings, I thought it is better to move this to a follow-up issue Properly run all CI tests for all crates instead of just the substrate-api-client #849.

…kspace

clangenb · 2025-05-01T08:56:40Z

.github/workflows/ci.yml

+                # Check build of stand-alone features.
+                cargo check --no-default-features -p substrate-api-client,
+                cargo check --no-default-features -p substrate-api-client --features sync-api,
+                cargo check --no-default-features -p substrate-api-client --features jsonrpsee-client,
+                cargo check --no-default-features -p substrate-api-client --features tungstenite-client,
+                cargo check --no-default-features -p substrate-api-client --features ws-client,
+                cargo check --no-default-features -p substrate-api-client --features staking-xt,
+                cargo check --no-default-features -p substrate-api-client --features contracts-xt,
+                cargo check --no-default-features -p substrate-api-client --features std,


When I moved the substreate-api-client to its own pure package crate the default behaviour changed to executed these checks for the whole workspace. I put this change now to preserve the current behaviour as some CI checks actually fail. I decided that it is better to fix this in a follow up issue as there are also some clippy warnings #849.

Yeah, I also stumbled upon this a while ago. I did not investigate the root cause exactly then, but we should fix the clippy issues 👍

clangenb · 2025-05-01T09:00:20Z

.github/workflows/ci.yml

+                cargo clippy -p substrate-api-client -- -D warnings,
+                cargo clippy -p substrate-api-client --no-default-features -- -D warnings,
+                cargo clippy -p substrate-api-client --all-features --examples -- -D warnings,


Clippy is not happy for the whole workspace yet. 😭 #849

clangenb · 2025-05-01T09:24:22Z

Do you have any idea about the license check? I added the necessary license to accepted, but it still fails. I can locally reproduce this, but I don't really know what to do.

haerdib · 2025-05-01T10:12:38Z

Do you have any idea about the license check? I added the necessary license to accepted, but it still fails. I can locally reproduce this, but I don't really know what to do.

Try it by accepting only the GPL3.0-or-later, maybe this works. Cargo about is not very robust..

clangenb · 2025-05-01T10:23:33Z

It complains that this does not exist... Shall we merge and create an issue?

haerdib · 2025-05-01T11:25:56Z

It complains that this does not exist... Shall we merge and create an issue?

Can you add the following accepted License? "GPL-3.0 WITH Classpath-exception-2.0" This should work.

clangenb · 2025-05-01T11:34:42Z

Thanks, worked!

haerdib

Cool, I like this change. But let's wait with merging until @Niederb has approved of it aswell. I'm not sure if he's currently working on something.

haerdib · 2025-05-01T11:48:16Z

api-client/src/api/api_client.rs

 use sp_core::Bytes;
 use sp_version::RuntimeVersion;

+pub const KSM_V14_METADATA_PATH: &str = "./../ksm_metadata_v14.bin";


haerdib · 2025-05-01T11:49:22Z

about.toml

    "Zlib",
-    "GPL-3.0"
+    "GPL-3.0",
+    "GPL-3.0 WITH Classpath-exception-2.0"


@Niederb You're more of an expert here: Is this an acceptable license for our keystore, or do we need to update our keystore license?

I don't think so. The exception makes it more liberal, so it should be fine.

Yes, the Classpath-exception makes it more liberal, so this is fine 👍

Niederb

I really like this change 👍 Less redundancies in the definition of the dependencies and versions

Niederb · 2025-05-05T14:12:04Z

examples/async/Cargo.toml


 # local deps
-substrate-api-client = { path = "../..", version = "1.17", features = ["staking-xt", "contracts-xt"] }
+substrate-api-client = { workspace = true, features = ["std", "jsonrpsee-client", "staking-xt", "contracts-xt"] }


More out of curiosity: Are the additional features "std", "jsonrpsee-client" neccessary? They are declared as default in the api-client/Cargo.toml no?

clangenb · 2025-05-06T06:19:49Z

Cargo.toml

+ac-compose-macros = { default-features = false, path = "compose-macros", version = "1.17" }
+ac-node-api = { default-features = false, path = "node-api", version = "1.17" }
+ac-primitives = { default-features = false, path = "primitives", version = "1.17" }
+substrate-api-client = { default-features = false, path = "api-client", version = "1.17" }


@Niederb regarding your #848 (comment). Yes the features are necessary, as we disable the default features here when we define the api-client as a workspace dependency.

We could also do it the other way around as we need the default-features except for the no-std tests, but I decided to be consistent with the rest.

Oh, I see. Makes sense then 👍

We could also explicitly put default-features = true in the async example to overwrite the workspace dependency setting. Not sure what is better though.

clangenb · 2025-05-06T06:20:33Z

Cool thanks for the reviews. You can merge the PR at your convenience then.

clangenb added 4 commits May 1, 2025 10:15

move api-client to its own crate and lift all dependencies to the wor…

2301d7f

…kspace

fix examples

3cb7bd9

fix std leak

6f945d2

taplo fmt

4daf0f9

clangenb requested review from Niederb and haerdib and removed request for haerdib May 1, 2025 08:38

clangenb added F5-refactor Does not change any functionality of the code E1-breaksnothing Q3-substantial F9-dependencies Pull requests that update a dependency file dependencies Pull requests that update a dependency file labels May 1, 2025

haerdib removed Q3-substantial dependencies Pull requests that update a dependency file labels May 1, 2025

clangenb added 3 commits May 1, 2025 10:47

fix cargo check

eaa8065

fmt

94e5e07

fix CI

5f559bf

clangenb mentioned this pull request May 1, 2025

Properly run all CI tests for all crates instead of just the substrate-api-client #849

Open

clangenb commented May 1, 2025

View reviewed changes

fix CI 2

f1d4e59

clangenb commented May 1, 2025

View reviewed changes

clangenb added 4 commits May 1, 2025 11:03

fix ac-examples-wasm

16a889a

fix balances in std

2a7cc43

taplo fmt

87c0707

add new accepted licence

22ccf8d

clangenb added 3 commits May 1, 2025 11:28

use workspace deps in ac-keystore

8b97f42

remove unnecessary deps

c629a7d

fix metadata path in test

8130660

clangenb added 5 commits May 1, 2025 11:38

fix tests

7765bc5

taplo fmt

ab6187f

fix clippy

b8c2824

fix metadata path 2

4110490

fix async testing test

a6e7169

fix licence check

f6100e0

haerdib approved these changes May 2, 2025

View reviewed changes

Niederb approved these changes May 6, 2025

View reviewed changes

clangenb commented May 6, 2025

View reviewed changes

Niederb merged commit 4b3d52e into master May 6, 2025
57 checks passed

Lift dependencies to workspace and move api-client to its own package #848

Lift dependencies to workspace and move api-client to its own package #848

Uh oh!

Conversation

clangenb commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clangenb May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haerdib commented May 1, 2025

Uh oh!

clangenb commented May 1, 2025

Uh oh!

haerdib commented May 1, 2025

Uh oh!

clangenb commented May 1, 2025

Uh oh!

haerdib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Niederb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clangenb commented May 1, 2025 •

edited

Loading

clangenb May 1, 2025 •

edited

Loading

clangenb commented May 1, 2025 •

edited

Loading

clangenb May 6, 2025 •

edited

Loading