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.

Sign up for GitHub

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

Jump to bottom

Inherit workspace dependency #994

Closed
ggwpez opened this issue Apr 8, 2023 · 4 comments
Closed

Inherit workspace dependency #994

ggwpez opened this issue Apr 8, 2023 · 4 comments
Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring.

Comments

@ggwpez
Copy link
Member

ggwpez commented Apr 8, 2023

Try out what Kilt did and use workspace dependencies to ease the version managment.
Example Cargo.toml and Workspace.

quote @bkchr

I would say we start with some like log etc. Deps that are shared between most crates and see how it goes.

Any known downsides with this @weichweich?
@weichweich
Copy link

@ggwpez It's nice to have everything in one place. Especially with none substrate/cumulus/polkadot dependencies. We for example had a different version for the same dependency in all crates. This doesn't happen anymore.

The only downside I noticed is that the feature management is somewhat unclear. You now have to check two places to be sure about which features are enabled for a crate.
I also think the intuition might be that the features specified on the workspace level are overwritten if features are specified in the crate, but the docs state that features are additive. So features enabled in the workspace are always on.

@ggwpez
Copy link
Member Author

ggwpez commented Apr 11, 2023

Thanks for the reply!
Not having multiple versions is probably the biggest improvement, from a management and compile time perspective.
Will take a look at the other things.

We probably want to do this after the mono-repo migration, I assume.

burdges referenced this issue in w3f/ring-vrf Apr 13, 2023
workspace dependency management
d989644 
We do not really have enough dependencies for this, but hey..
via https://github.com/paritytech/substrate/issues/13859
@gilescope
Copy link
Contributor

Yeah no reason to slow down the mono-repo switch to wait for this and I think it will be nicer to do once we're all in one workspace. Jan said there were a couple of crates that absolutely had to be the same version or things didn't work (wasmtime might have been one?).

@ggwpez ggwpez changed the title Workspace dependency management Inherit workspace dependency Aug 14, 2023
@ggwpez ggwpez transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I4-refactor Code needs refactoring. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. and removed I7-refactor labels Aug 25, 2023
@ggwpez ggwpez mentioned this issue Oct 28, 2023
Merged
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
@boundless-forest
Only one EthBlockGasLimit for the ts tests (paritytech#994)
6840b20 
* Use correct block gas limit

* Format ts

* Force push to fix ci

* Fix conflicts

* Format
github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
@ggwpez
Lift dependencies to the workspace (Part 1) (#2070)
e80c247 
Changes (partial #994):
- Set log to `0.4.20` everywhere
- Lift `log` to the workspace

Starting with a simpler one after seeing
#2065 from @jsdw.
This sets the `default-features` to `false` in the root and then
overwrites that in each create to its original value. This is necessary
since otherwise the `default` features are additive and its impossible
to disable them in the crate again once they are enabled in the
workspace.

I am using a tool to do this, so its mostly a test to see that it works
as expected.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@bkchr bkchr mentioned this issue Feb 16, 2024
Merged
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
@ggwpez
Lift dependencies to the workspace (Part 1) (paritytech#2070)
aa7d53a 
Changes (partial paritytech#994):
- Set log to `0.4.20` everywhere
- Lift `log` to the workspace

Starting with a simpler one after seeing
paritytech#2065 from @jsdw.
This sets the `default-features` to `false` in the root and then
overwrites that in each create to its original value. This is necessary
since otherwise the `default` features are additive and its impossible
to disable them in the crate again once they are enabled in the
workspace.

I am using a tool to do this, so its mostly a test to see that it works
as expected.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Sep 20, 2024

Done

@ggwpez ggwpez closed this as completed Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@gilescope @ggwpez @weichweich @the-right-joyce