dfinity / sdk

IC SDK: a Software Development Kit for creating and managing canister smart contracts on the ICP blockchain.
https://internetcomputer.org/developers
Apache License 2.0
174 stars 83 forks source link

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

Closed anchpop closed 3 months ago

anchpop commented 3 months ago

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 commented 3 months ago

Thanks @ericswanson-dfinity , I added an automated test