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

feat: rebuild only necessary canisters #3710

Open
wants to merge 366 commits into
base: master
Choose a base branch
from

Conversation

vporton
Copy link
Contributor

@vporton vporton commented Apr 14, 2024

Description

DFX doesn't compile canisters for which all dependencies are elder than the .wasm file. This results in big compilation speedups.

The PR also adds tracing and comments intended for future improvement of compilation speed.

This PR is a WIP. There is potential for further speedup and there is missing logging. More testing is necessary.

How Has This Been Tested?

I run it before the .wasm file has been created and after it with a significant speedup. Also dfx build -vv correctly does not output any moc compilation logs, when invoking dfx build for the second time.

Later I did bats make_like.bash, add_dependency.bash, and broken_canister_dep.bash automated testing.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@vporton
Copy link
Contributor Author

vporton commented Apr 14, 2024

dfx deploy of my current DFX version running in https://github.com/vporton/zondirectory2 repo causes an infinite loop (but dfx build --all doesn't).

@vporton
Copy link
Contributor Author

vporton commented Apr 16, 2024

The last commit (79faa772dd3b8a7683ed9670a1447ecd0b2893ba) seems working!

Also note that unlike the original (master) code, my code does recompile dependent canisters (canister:* dependencies) when a dependency changed.

The code needs refactoring and more serious testing.

@vporton
Copy link
Contributor Author

vporton commented Apr 16, 2024

Testing shows a bug: When specifying a dependency to build: dfx build dependency, the dependent canister may recompile, too.

@vporton
Copy link
Contributor Author

vporton commented Apr 16, 2024

The latest version commit 030f9af902515edd70d60acde5d2cb547c200b40 works as expected on simple (manual) tests (with more tests than the past time).

You may try to use it for your home projects, but don't try it for mission-critical tasks or if you fear to be lost in wrong recompilations, because errors are still possible.

@vporton
Copy link
Contributor Author

vporton commented Apr 16, 2024

Fixed crash on dfx build --all (creates a new bug, about compiling non-Motoko canister: dependencies).

@vporton
Copy link
Contributor Author

vporton commented May 10, 2024

I've fixed the issue of calling moc --print-deps more than once for a given file.
However a small issue remains: it outputs Building dependencies graph. twice on dfx deploy.

No other remaining issues. What's about code review?

@ericswanson-dfinity
Copy link
Member

I've fixed the issue of calling moc --print-deps more than once for a given file. However a small issue remains: it outputs Building dependencies graph. twice on dfx deploy.

No other remaining issues. What's about code review?

These are the kinds of issues I see that indicate the work is not yet ready for code review:

  • numerous // TODO
  • commented-out code
  • panic!("programming error");
  • error checking with assert! rather than returning an error

It also adds a new feature, deploy: bool for canisters, that we haven't discussed and should be first submitted as a separate PR anyway.

@vporton vporton mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants