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

depend-o-pocalipse #9450

Merged
merged 29 commits into from
Aug 13, 2021
Merged

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Jul 28, 2021

Fortune favors the brave.

There seem to be a few dependencies that I can get it to compile without it needing them.

Todo:

  • unused dependencies removed
  • unused dev dependencies included.
  • green build
  • check cumulus build
  • check frame-benchmarking
  • check try-runtime
  • mention on community channel

@gilescope gilescope added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 28, 2021
bin/node/bench/Cargo.toml Outdated Show resolved Hide resolved
@gilescope gilescope changed the title WIP: depend-o-pocalipse depend-o-pocalipse Jul 29, 2021
@gilescope gilescope added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 29, 2021
@gilescope gilescope requested a review from gnunicorn July 29, 2021 17:11
@shawntabrizi
Copy link
Member

How do you identify these? Some tool/script?

@gilescope
Copy link
Contributor Author

Yup, cargo metadata on the workspace, then cargo edit's cargo rm on each dep and cargo check --all-targets to see if it built afterwards. record good results to a file and then git reset --hard and try a different dep.

It ran all night but it seemed to not do too bad a job.... The down side is I don't have enough color to know if there's any reason we can't remove these dependencies. This is where I need your help to point out anything deeply suspicious - "surely that crate's used by x" etc.

I'm going to have a follow up PR that does the same but for dev-dependencies but I want to land them separately.
(We can brush up the function into something nice and put it into cargo unleashed if it is useful to people - there's already a subcommand in cargo-unleash that does something in this direction but isn't quite as brute force.)

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Neat. Nothing too suspicious coming up glancing over them, there's a good chance these are indeed not needed (anymore).

But two questions on your process, just to make sure we don't accidentally dropped one too many:

  1. did you build in release-mode(, too)?
  2. have the frame/pallets been build for wasm/no_std?

@gilescope
Copy link
Contributor Author

gilescope commented Jul 30, 2021 via email

@gilescope
Copy link
Contributor Author

Release build is clean. I'm unclear at the moment how to build just the wasm for the whole workspace.

@gui1117
Copy link
Contributor

gui1117 commented Aug 4, 2021

The pallet/frame crate should be good if a release build of node-runtime is done.

gui1117
gui1117 previously approved these changes Aug 4, 2021
frame/contracts/Cargo.toml Outdated Show resolved Hide resolved
@gilescope gilescope added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Aug 6, 2021
@gui1117 gui1117 dismissed their stale review August 6, 2021 12:18

I have to look again once done

@gilescope
Copy link
Contributor Author

self_destruct_works test failure is a bit of a problem. If I do rm Cargo.lock; rm ../../Cargo.lock; cargo clean; cargo test self_destruct from frame/contracts I can reproduce this test failure on master and master a couple of months back. Possibly what's happening is a change in the Cargo.lock changes the dependencies and a different rent is applied? It doesn't seem to have anything to do with this branch.

@gilescope
Copy link
Contributor Author

Ok test failure was just due to later wat crate being included in Cargo.lock leading to slight increase in wasm size leading to increased rent leading to test failure.

@athei
Copy link
Member

athei commented Aug 11, 2021

Yup, cargo metadata on the workspace, then cargo edit's cargo rm on each dep and cargo check --all-targets to see if it built afterwards. record good results to a file and then git reset --hard and try a different dep.

Did you try cargo-udeps?

@gilescope
Copy link
Contributor Author

I did try udeps but it wasn't that helpful for me. I coded up something that was very dumb (and inefficent) but was reasonably effective because it didn't try and do anything fancy.

@gilescope
Copy link
Contributor Author

(There's some auto-formatting of the toml files going on here which is nice, though unclear how vscode is managing this?)

@gilescope gilescope added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 12, 2021
@gilescope
Copy link
Contributor Author

gilescope commented Aug 12, 2021

Aside from the above no other .rs has been changed - the others are just cargo fmt. (Potentially we should pin the compiler via a rust-toolchain.toml so that we manage when we take these rustfmt changes)

@gilescope gilescope merged commit e4a0cc4 into paritytech:master Aug 13, 2021
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. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants