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.

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

Switch to workspace dependencies #664

Merged
merged 8 commits into from
Aug 22, 2023
Merged

Switch to workspace dependencies #664

merged 8 commits into from
Aug 22, 2023

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Apr 22, 2023

Closes #610

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Apr 23, 2023

As you can see, CI is red, and the builds are broken. This is without any changes to the Cargo.lock, so it must that the features are resolving differently somehow.
I'll investigate this later, but for now, @dmitrylavrenov if you can spot the root cause of this at a glance (i.e. without too much effort) please let me know.

@dmitrylavrenov
Copy link
Contributor

dmitrylavrenov commented Apr 24, 2023

As you can see, CI is red, and the builds are broken. This is without any changes to the Cargo.lock, so it must that the features are resolving differently somehow.
I'll investigate this later, but for now, @dmitrylavrenov if you can spot the root cause of this at a glance (i.e. without too much effort) please let me know.

As I figured out workspace maintains the additive nature of the package dependency, it can enable default features but it can't disable them:

Already pushed the commit with additive nature.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Apr 24, 2023

I see what the problem is. I don't like the idea of having default features disabled for some of the workspace deps, but enabled for the rest. How about we disbale default features for all workspace deps, and the selectively enable them as needed?

@dmitrylavrenov
Copy link
Contributor

I see what the problem is. I don't like the idea of having default features disabled for some of the workspace deps, but enabled for the rest. How about we disbale default features for all workspace deps, and the selectively enable them as needed?

Agree with you, it should avoid having confusion in mind.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Apr 24, 2023

@dmitrylavrenov thx for the help, I'll take care of the rest.

@dmitrylavrenov dmitrylavrenov force-pushed the wsdeps branch 4 times, most recently from 23cccd1 to 88f7ca4 Compare May 8, 2023 16:18
@dmitrylavrenov
Copy link
Contributor

@MOZGIII default features have beet set to false for all deps.
Please, check.

Copy link
Contributor Author

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

I think we must also run local tests for this to be sure (to see if we maybe disabled some features unexpectedly that don't break the compilation, but reduce the effective feature set for instance). Should be alright though.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented May 19, 2023

Status update: this can be merged, but we decided to introduce snapshotting of the active feature per dependency, land that at CI so we get an alert when features change, and then ensure this PR doesn't cause a snapshot change to rnsure we only purely rewrite the way we declare the features, and bit the actual sets of features.
@dmitrylavrenov please send an update on the progress of the specifics here once you have a chance

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 1, 2023

See #687

The diff for the current branch is 7bbb1bd - I think it is acceptable.
It is also interesting to check the diff for my initial attempt - aa0c61a
And the diff between my first attempt and your additions - cac350a

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Jul 30, 2023

#687 is getting merged now, we can continue here

@dmitrylavrenov
Copy link
Contributor

@MOZGIII Just merged master and resolved conflict. I guess it can be reviewed now.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Aug 17, 2023

Can we make it so the features snapshot is unchanged?

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Aug 18, 2023

I suggest that we squash the commits in this PR and rebase on top of master so it's easier to achieve the changeset we are looking for here - which is to switch to the workspace dependencies without changes to the features snapshot. This is going to be non-trivial, especially with the current state of this PR, however, it is doable.

@dmitrylavrenov
Copy link
Contributor

I suggest that we squash the commits in this PR and rebase on top of master so it's easier to achieve the changeset we are looking for here - which is to switch to the workspace dependencies without changes to the features snapshot. This is going to be non-trivial, especially with the current state of this PR, however, it is doable.

Spent time to properly rebase. Current features snapshot changes are related to default-features = false usage based on

How about we disbale default features for all workspace deps, and the selectively enable them as needed?

@dmitrylavrenov
Copy link
Contributor

@MOZGIII Check again, please. Features snapshot hasn't been changed.

Copy link
Contributor Author

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Looks great! I'll take some time to review to make sure I understand the tradeoffs taken here.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Aug 21, 2023

It would be nice to add a clippy lint to only allow workspace dependencies. This doesn't look too difficult, check out this for a reference: https://github.com/rust-lang/rust-clippy/tree/master/clippy_lints/src/cargo

@MOZGIII MOZGIII added this pull request to the merge queue Aug 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 22, 2023
@MOZGIII MOZGIII merged commit 52f829d into master Aug 22, 2023
18 checks passed
@MOZGIII MOZGIII deleted the wsdeps branch August 22, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Switch to workspace dependencies for substrate and frontier forks
2 participants