Skip to content

fix: Build the canister dependency graph only for the requested canisters - #3818

Merged
anchpop merged 2 commits into
masterfrom
@anchpop/more-permissive-dependency-search
Jul 5, 2024
Merged

fix: Build the canister dependency graph only for the requested canisters#3818
anchpop merged 2 commits into
masterfrom
@anchpop/more-permissive-dependency-search

Conversation

@anchpop

@anchpop anchpop commented Jul 3, 2024

Copy link
Copy Markdown
Contributor

How Has This Been Tested?

I created a test project called hello and added it to my dfx.json. Then I added

    "malformed": {
      "remote": {
        "id": {
          "local": "hptcf-emaaa-aaaaa-qaawq-cai"
        }
      }
    }

under "canisters" in dfx.json. Here is the result of running dfx deploy hello on dfx 0.20.1:

❯ dfx deploy hello                                                                                              
Deploying: hello
All canisters have already been created.
Building canisters...
WARN: Canister 'malformed' has no .did file configured.
WARN: .did file for canister 'hello_assets' does not exist.
Error: Failed while trying to deploy canisters.
Caused by: Failed to build all canisters.
Caused by: Failed while trying to build all canisters.
Caused by: Failed while trying to build all canisters in the canister pool.
Caused by: Failed to build dependencies graph for canister pool.
Caused by: Failed to get dependencies for canister 'malformed'.
Caused by: Failed to create <Type>CanisterInfo for canister 'malformed'.
Caused by: `main` attribute is required on Motoko canisters in dfx.json

You can see how adding this entry to dfx.json has caused dfx deploy hello to fail for no apparent reason. You even get a weird message complaining about motoko despite never having mentioned motoko in the entry for malformed.

Here is the result with a version of dfx I just built from this branch:

❯ ./dfx deploy hello
Deploying: hello
All canisters have already been created.
Building canisters...
WARN: Canister 'malformed' has no .did file configured.
WARN: .did file for canister 'hello_assets' does not exist.
  Installed dfx 0.21.0-beta.0+rev13.dirty-ae5c7f33 to cache.
Installing canisters...
Installing code for canister hello, with canister ID bkyz2-fmaaa-aaaaa-qaaaq-cai
Deployed canisters.
URLs:
  Frontend canister via browser
    hello_assets:
      - http://127.0.0.1:8000/?canisterId=bd3sg-teaaa-aaaaa-qaaba-cai
      - http://bd3sg-teaaa-aaaaa-qaaba-cai.localhost:8000/
  Backend canister via Candid interface:
    hello: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=bkyz2-fmaaa-aaaaa-qaaaq-cai
    sns_governance: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=be2us-64aaa-aaaaa-qaabq-cai
    sns_index: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=br5f7-7uaaa-aaaaa-qaaca-cai
    sns_ledger: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=bw4dl-smaaa-aaaaa-qaacq-cai
    sns_root: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=b77ix-eeaaa-aaaaa-qaada-cai
    sns_swap: http://127.0.0.1:8000/?canisterId=avqkn-guaaa-aaaaa-qaaea-cai&id=by6od-j4aaa-aaaaa-qaadq-cai

Now, the deployment succeeds

Description

Build the canister dependency graph only for the requested canisters, instead of building it for all canisters.

The motivation for this is that sometimes canister.builder.get_dependencies returns an error for some canisters, as those canisters are not properly formed (e.g. they are Motoko canisters with no main field). In this case, you could still theoretically call dfx deploy <canister> and you might expect dfx to not complain that some unrelated canisters in your dfx.json are not properly formed. By only calling canister.builder.get_dependencies on canisters that are actually needed for building the canister being deployed, we can avoid this from happening.

Of course, dfx deploy without specifying a canister will still not work. But it seems to be common for people to use dfx deploy <canister> this way, as both I and @dskloetd have run into this before and that is just the cases I know about. And I think it is not a great user experience to have dfx deploy <canister> not work in this case when it easily could.

I would also like to add an automated test but I'm not sure how.

Checklist:

@anchpop
anchpop requested a review from a team as a code owner July 3, 2024 23:01
@anchpop anchpop changed the title fix: Build the canister dependency graph lazily fix: Build the canister dependency graph only for the requested canisters Jul 3, 2024

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here is an example test that should be easy to adapt to verify the changes in this PR:

https://github.com/dfinity/sdk/blob/master/e2e/tests-dfx/deploy.bash#L152

@anchpop
anchpop force-pushed the @anchpop/more-permissive-dependency-search branch 2 times, most recently from d8bdd27 to 8cb81bb Compare July 5, 2024 20:02
@anchpop

anchpop commented Jul 5, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @ericswanson-dfinity , I added an automated test

@anchpop
anchpop force-pushed the @anchpop/more-permissive-dependency-search branch from 8cb81bb to eb41edc Compare July 5, 2024 20:18
@anchpop
anchpop enabled auto-merge (squash) July 5, 2024 21:03
@anchpop
anchpop merged commit 2ce92fa into master Jul 5, 2024
@anchpop
anchpop deleted the @anchpop/more-permissive-dependency-search branch July 5, 2024 21:06
anchpop added a commit to dfinity/snsdemo that referenced this pull request Aug 8, 2024
Hopefully we can remove it from the SNS cli soon. Removing it from here
means that won't be an issue. Thanks to
dfinity/sdk#3818, it no longer serves any
purpose in this repository anyway
rikonor pushed a commit that referenced this pull request Feb 3, 2025
…ters (#3818)

* Make dependency search more permissive to malformed canisters

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant