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

Refactor: Use workspace dependencies #1725

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Refactor: Use workspace dependencies #1725

merged 5 commits into from
Feb 12, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Feb 8, 2024

Description

This PR move previous @wischli work (1a45bbe) to its own PR, and extends it to remove any hardcoded path from our codebase in Cargo.toml files.

Changes

  • No more paths in any Cargo.toml file except the in the root file.
  • Use workspace definitions for [package].
  • Removed some unused dependencies.
  • Fixed some dependency features

@lemunozm lemunozm added P5-soon Issue should be addressed soon. I6-refactoring Code needs refactoring. labels Feb 8, 2024
@lemunozm lemunozm self-assigned this Feb 8, 2024
@lemunozm lemunozm marked this pull request as draft February 8, 2024 11:53
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Correcting my own left overs (probably): Some crates still don't fully make use of workspace inheritance. I marked all my findings. Thanks for extracting this already!

libs/types/Cargo.toml Outdated Show resolved Hide resolved
pallets/pool-registry/Cargo.toml Outdated Show resolved Hide resolved
pallets/pool-system/Cargo.toml Outdated Show resolved Hide resolved
pallets/pool-system/Cargo.toml Outdated Show resolved Hide resolved
pallets/restricted-tokens/Cargo.toml Show resolved Hide resolved
pallets/keystore/Cargo.toml Outdated Show resolved Hide resolved
pallets/loans/Cargo.toml Show resolved Hide resolved
pallets/permissions/Cargo.toml Outdated Show resolved Hide resolved
pallets/crowdloan-claim/Cargo.toml Outdated Show resolved Hide resolved
pallets/migration/Cargo.toml Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

lemunozm commented Feb 8, 2024

I forgot to tell that this is still very WIP. I need to update a lot of dependencies

@lemunozm
Copy link
Contributor Author

lemunozm commented Feb 8, 2024

Thanks for the reminders @wischli!

@lemunozm
Copy link
Contributor Author

lemunozm commented Feb 9, 2024

The compilation state after this refactor has not changed (manually tested):

  • Checking process (no std):
    • cargo check
    • cargo check -F runtime-benchmarks
      • It fails with a missing feature issue, but the same issue already appears in main. I think it's because fudge is not setting the correct features (runtime-benchmarks & try-runtime) for its dependencies. Excluding integration tests makes the compilation work.
    • cargo check --workspace -F runtime-benchmarks --exclude runtime-integration-tests
    • cargo check -F try-runtime
    • cargo check -F runtime-benchmarks,try-runtime
      • Same issue as before. Also failing in main
  • Testing process (std):
    • cargo test
    • cargo test -F runtime-benchmarks
      • Same issue as before. Also failing in main
    • cargo test --workspace -F runtime-benchmarks --exclude runtime-integration-tests
    • cargo test -F try-runtime
    • cargo test -F runtime-benchmarks,try-runtime
      • Same issue as before. Also failing in main

I will merge this PR with the above errors (already in main), but I would try to fix them before moving to polkadot-1.1.0.

@lemunozm lemunozm marked this pull request as ready for review February 9, 2024 15:53
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

I really appreciate that you checked all the compiling possibilities with this and the main branch. I really hope we can fix all of that with the Polkadot v1.1.0 upgrade!

This PR looks super clean and ready to be merged to me 🚀

@lemunozm lemunozm enabled auto-merge (squash) February 9, 2024 23:30
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Thanks for the cleaning!


orml-traits = { git = "https://github.com/moonbeam-foundation/open-runtime-module-library", branch = "moonbeam-polkadot-v0.9.43", default-features = false }
# TODO(william): Used to be "https://github.com/moonbeam-foundation/open-runtime-module-library", branch = "moonbeam-polkadot-v0.9.43"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - do we still need this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for this PR, it comes from a cherry-pick from #1605

Good eye!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, just realising that I skipped to double-check this pallet crate because it was under gateway and it still contains paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But they're local so not big issue

@lemunozm lemunozm merged commit f4f4bcc into main Feb 12, 2024
9 checks passed
@lemunozm lemunozm deleted the workspace-deps branch February 12, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I6-refactoring Code needs refactoring. P5-soon Issue should be addressed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants