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

Replace ic-cdk-optimizer with ic-wasm-shrink #2868

Merged
merged 7 commits into from
Jul 17, 2023
Merged

Replace ic-cdk-optimizer with ic-wasm-shrink #2868

merged 7 commits into from
Jul 17, 2023

Conversation

bitdivine
Copy link
Member

@bitdivine bitdivine commented Jul 12, 2023 •
edited
Loading

Motivation

ic-cdk-optimizer has been deprecated for a long time. However, as the replacement did not support our use cases we could not migrate to the proposed replacement tools. Now two things encourage the migration:

  • There is now a drop-in replacement in the form of ic-wasm shrink
  • We cannot make some changes to the Rust code, because ic-cdk-optimizer does not support the corresponding opcodes.

Changes

  • Replace ic-cdk-optimizer with ic-wasm shrink

Tests

  • CI should check correctness.

Todos

  • Add entry to changelog (if necessary).
  • The wasm size needs to be compared before and after this change.

@bitdivine bitdivine marked this pull request as draft July 12, 2023 09:39
@peterpeterparker
Copy link
Member

Just in case useful, in addition to shrink, I also added an extra step for the metadata when I migrated Juno from ic-cdk-optimizer to ic-wasm: https://github.com/buildwithjuno/juno/pull/137/files

It's something I cloned from II. PRs dfinity/internet-identity#1370 (ic-wasm) and https://github.com/dfinity/internet-identity/pull/1373/files (metadata)

@bitdivine
Copy link
Member Author

@peterpeterparker Thanks for the watchful eye. We actually already have that, and the git commit, in the metadata-add command. https://github.com/dfinity/nns-dapp/blob/main/scripts/dfx-wasm-metadata-add

@bitdivine bitdivine marked this pull request as ready for review July 17, 2023 07:43
@bitdivine
Copy link
Member Author

bitdivine commented Jul 17, 2023 •
edited
Loading

Note: I would really also like to check the cycle count but there isn't a good way of doing this fast. I have started making a toy state but creating a realistic state and counting cycles is going to be quite a bit of work. It is probably better to step back for a moment because something that takes a lot of work needs to pay for itself over many PRs. Maybe it makes sense to get in touch with people who have replicated mainnet state before, for example.

I have created an issue to that effect here: https://dfinity.atlassian.net/browse/GIX-1729

peterpeterparker
peterpeterparker approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

lmuntaner
lmuntaner approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

LGTM, one question

@bitdivine bitdivine enabled auto-merge July 17, 2023 09:10
@bitdivine bitdivine added this pull request to the merge queue Jul 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 17, 2023
@bitdivine bitdivine enabled auto-merge July 17, 2023 15:49
@bitdivine bitdivine added this pull request to the merge queue Jul 17, 2023
Merged via the queue into main with commit 4b99d56 Jul 17, 2023
@bitdivine bitdivine deleted the rm-ic-cdk-optimizer branch July 17, 2023 16:51
ggreif
ggreif reviewed Jul 19, 2023
View reviewed changes
e2e-tests/scripts/nns-canister.Dockerfile
@@ -7,7 +7,8 @@ RUN apt -yq update && \
rm -rf /tmp/* /var/lib/apt/lists/* /var/tmp/*

RUN curl -L --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/cargo-bins/cargo-binstall/181b5293e73cfe16f7a79c5b3a4339bd522d31f3/install-from-binstall-release.sh | bash && cargo binstall -V
RUN cargo binstall --no-confirm --version 0.3.2 ic-cdk-optimizer
# Note: The version should match dfx.json, however it is expected that the old e2e tests, including this file, will be deleted soon so there is no point in adding the code to get the version.
RUN cargo binstall --no-confirm "[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an outdated version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggreif ggreif ggreif left review comments

@peterpeterparker peterpeterparker peterpeterparker approved these changes

@lmuntaner lmuntaner lmuntaner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bitdivine @peterpeterparker @ggreif @lmuntaner