Implement new commands `build` and `check` + introduce bundles (.contract files) by cmichi · Pull Request #97 · use-ink/cargo-contract

cmichi · 2020-11-02T20:02:29Z

Closes #96 by implementing

a new command cargo contract build, which results in a target/metadata.json, a target/<name>.wasm, and a target/<name>.contract (the .pack file mentioned in the issue).
a new command cargo contract check, which just tries to build the contract without generating metadata or optimizing the Wasm.
deprecates cargo contract generate-metadata for cargo contract build (which implicitly does this or via --metadata-only skips unnecessary operations not strictly needes for generating metadata).

ascjones

What do you think about giving the resulting file a different name/extension than just metadata.json, so that we know we are dealing with a packed file? Might be useful for the canvas-ui to know to expect a packed file. @Robbepop?

src/cmd/metadata.rs

athei

I think generating that pack is the default case. We should make that as easy as possible. Imho the cli should look like this:

There is only the build command which does what pack does today. It has two options skip-code and skip-metadata. If either of those is specified we also skip the generation of the pack. Specifying both is an error (obviously).

src/cmd/metadata.rs

src/main.rs

ascjones · 2020-11-04T10:06:29Z

I think generating that pack is the default case. We should make that as easy as possible. Imho the cli should look like this:

There is only the build command which does what pack does today. It has two options skip-code and skip-metadata. If either of those is specified we also skip the generation of the pack. Specifying both is an error (obviously).

I like this idea, since in most cases you would want to do both - and incremental compilation means that even compiling both will not cost too much after the first time.

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

cmichi · 2020-11-05T05:21:30Z

There is only the build command which does what pack does today. It has two options skip-code and skip-metadata. If either of those is specified we also skip the generation of the pack. Specifying both is an error (obviously).

So I also like it, but I'm not sure about the --skip-code. As I understand it, @athei meant it as "not generating the Wasm blob", but in order to generate the metadata we need to build that blob anyway. So there's no cost in outputting the file name to that one as well.

What I suggest instead is to have the skip-metadata like you suggested and then a skip-packing:

--skip-metadata    Only the Wasm is created, generation of metadata and a packed .contract file is skipped
--skip-packing     Only the Wasm and the metadata are generated, no packed .contract file is created

I've added a commit which does that. Lmk what you think.

athei

You are right. --skip-code makes no sense. My only complaint is that we should not use the word "packing". I think "bundle" better expresses what we are doing. "Pack" could also mean some kind of compression which is not what we are doing here. I mean we could apply compression on top but the main thing we are doing is still "bundle".

src/main.rs

Robbepop · 2020-11-05T09:47:26Z

src/main.rs

+        #[structopt(long = "skip-packing", conflicts_with = "skip-metadata")]
+        skip_packing: bool,
+        /// Only the Wasm is created, generation of metadata and a packed .contract file is skipped
+        #[structopt(long = "skip-metadata", conflicts_with = "skip-packing")]
+        skip_metadata: bool,


Can we please name things positively instead of negatively? I.e. bundle or bundle_contract or build_bundle instead of skip_bundle? Same for skip_metadata.

While I agree with the general sentiment of prioritizing positive naming, I'm not sure how we can do that here, since default behavior is wasm+metadata+bundle and we want to provide a possibility to leave certain things out.

So we need better flag names for

--skip-metadata Only the Wasm is created, generation of metadata and a packed .contract file is skipped --skip-packing Only the Wasm and the metadata are generated, no packed .contract file is created

Hmm, maybe --only-wasm and --only-metadata would be better?

Or do you suggest changing default behavior?

Why do we need these flags in the first place? I think this should be automated just like you don't need to specify to cargo which rust files to build.

If your contract implementation has changed you always want to build the Wasm AND if your interfaces have changed you always want to build metadata. This already eliminates the wasm flag because bundling pretty much always involved building the wasm. So the only question is: Do we want to always build metadata? Do we want to infer it? And if so, how? Or do we want to make it skip-opt-in as proposed so far?

I don't follow, can you elaborate?

We changed default behavior of cargo contract build to generate wasm+metadata+pack. And now we want to provide flags to skip certain steps. Which part of that can be automated?

I estimate that those commands will be used most frequently:

cargo contract build (very often)

cargo contract bundle (rarely)

cargo contract metadata (nearly never)

So we could also remove cargo contract metadata and add a --skip-metadata flag to cargo contract bundle and keep cargo contract build. Building only metadata seldomly makes real sense.

You kind of can compare cargo contract bundle with cargo publish. Also ideally cargo contract bunlde should do checks similar to cargo publish like: Do contract authors exists and are they well formatted?

What is the motivation to change behaviour of cargo contract build instead of adding yet another command cargo contract bundle?

See earlier discussion in this PR: #97 (review)

So we could also remove cargo contract metadata and add a --skip-metadata flag to cargo contract bundle and keep cargo contract build. Building only metadata seldomly makes real sense.

So what this PR already does is that it removes the cargo contract generate-metadata cmd, makes cargo contract build build everything by default, and adds skip flags for omitting metadata/bundling.

Then using the most frequently used command forces users to type in cargo contract build --skip-metadata which is messy imo. Generating metadat by default is bad here because it adds tons of overhead and users will question ink! with taking 2 minutes to do a fresh build for a hello world contract. We should focus to make the most frequent command the simplest to use:

Most frequent: cargo contract build

Less frequent: cargo contract bundle or cargo contract bundle --skip-metadata

Least frequent: cargo contract generate-metadata

Building a bundle without updating the .wasm is pretty much never what you want so there should be no option available to do this.

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

Robbepop · 2020-11-05T10:24:38Z

I think generating that pack is the default case. We should make that as easy as possible. Imho the cli should look like this:

There is only the build command which does what pack does today. It has two options skip-code and skip-metadata. If either of those is specified we also skip the generation of the pack. Specifying both is an error (obviously).

Building the pack should not be the default case since it incurs tons of overhead due to building .wasm AND metadata basically doubling the compile times.

athei · 2020-11-05T10:29:45Z

Isn't that what incremental compilation is for? With metadata not using link time optimization it will not add much time. I predict that people just want a new *.contract file to upload almost always. We should optimize for this case. Who will fumble with two different files and think about what to rebuild in order to save a a second to skip metadata build?

Robbepop · 2020-11-05T10:31:25Z

Isn't that what incremental compilation is for? With metadata not using link time optimization it will not add much time. I predict that people just want a new *.contract file to upload almost always. We should optimize for this case. Who will fumble with two different files and think about what to rebuild in order to save a a second to skip metadata build?

Have you tried building both cargo contract build and cargo contract generate-metadata? They build all ink! crates with different crate features and therefore cannot reuse a lot of cached build artifacts from one another. Also generate-metadata even builds more stuff generally so tends to be even more of a doubling of fresh build time in general.

athei · 2020-11-05T10:33:49Z

They cannot use the cache from one another but their own cache from a previous run, don't they?

Robbepop · 2020-11-05T10:44:19Z

They cannot use the cache from one another but their own cache from a previous run, don't they?

Yes they can but still build times are going to be significantly worse when making bundling and all of its extra steps the default.

On my system these are the benchmarks for:

cargo contract build (fresh): 40 seconds
cargo contract generate-metadata (fresh): 120 seconds
cargo contract build (incremental): 2.65 seconds
cargo contract generate-metadata (incremental): 7.39 seconds
cargo contract build (no-op): 0.8 seconds
cargo contract generate-metadata (no-op): 2.26 seconds

Incremental builds are measured after touch src/lib.rs. No-op builds are measured not changing anything in between.

From my evaluation of these results merging cargo contract build and cargo contract generate-metadata by default is a bad idea. Pushing the most frequent operation cargo contract build from 2.65 seconds to over 10 seconds, nearly x4. So I lied when I said it is x2 because it is actually way worse.

cmichi · 2020-11-05T10:48:18Z

@Robbepop Why do you think generating a wasm without metadata ist the most common operation a user wants to do? My impression was that we nearly always mention the line cargo contract build && cargo contract generate-metadata in docs/tutorials/etc..

Robbepop · 2020-11-05T10:51:29Z

@Robbepop Why do you think generating a wasm without metadata ist the most common operation a user wants to do? My impression was that we nearly always mention the line cargo contract build && cargo contract generate-metadata in docs/tutorials/etc..

Good question!
I compare cargo contract build mostly with cargo check and cargo contract bundle mostly with a composition of cargo build and/or cargo publish. You need cargo contract build in order to check if your contract compiles successfully on Wasm, you need cargo test to build tests, you basically never need cargo contract generate-metadata and you are going to need cargo contract bundle if you plan to deploy on-chain. Note that cargo contract bundle is comparable to cargo publish because unlike cargo build it will also have some additional checks to verify that no important metadata information is missing for deployment.

cmichi · 2020-11-09T08:14:22Z

All comments addressed. Output now:

$ cargo run -- contract build --manifest-path ../ink/examples/flipper/Cargo.toml                                                                               
   Compiling cargo-contract v0.7.1 (/home/michi/projects/cargo-contract)                                                                                       
    Finished dev [unoptimized + debuginfo] target(s) in 15.97s                                                                                                 
     Running `target/debug/cargo-contract contract build --manifest-path ../ink/examples/flipper/Cargo.toml`                                                   
 [1/5] Building cargo project                                                  
    Finished release [optimized] target(s) in 0.09s                                                                                                            
 [2/5] Post processing wasm file                                               
 [3/5] Optimizing wasm file                                                    
    Updating crates.io index                                                   
   Compiling metadata-gen v0.1.0 (/tmp/cargo-contract_NFmb1e/.ink/metadata_gen)                                                                                
    Finished release [optimized] target(s) in 2.59s                                                                                                            
     Running `target/release/metadata-gen`                                                                                                                     
 [4/5] Generating bundle                                                                                                                                       
 [5/5] Generating metadata                                                     
                                                                                                                                                               
Original wasm size: 19.1K, Optimized: 2.5K                                                                                                                     
                                                                               
Your contract artifacts are ready. You can find them in:                       
/home/michi/projects/ink/examples/flipper/target    
                           
  - flipper.contract (code + metadata)                                         
  - flipper.wasm (the contract's code)                                                                                                                         
  - metadata.json (the contract's metadata)

ascjones

Nice, like the new output. Just added a couple of extra comments

src/cmd/metadata.rs

src/cmd/build.rs

metadata/lib.rs

src/cmd/metadata.rs

ascjones

LGTM. I'd like either @Robbepop or @athei to give it a ✔️ before merging

athei · 2020-11-09T14:51:31Z

I updated the branch and built a fresh flipper contract. The output seems to be wrong again:

The "Generating metadata" is at the wrong position. It should be directly behind step [3/5].

alexander:~% cargo contract build
 [1/5] Building cargo project
   ...
   Compiling flipper v0.1.0 (/var/folders/8g/_rkmq65n0qnf70c41xvp174h0000gn/T/cargo-contract_qOrMhF)
    Finished release [optimized] target(s) in 23.10s
 [2/5] Post processing wasm file
 [3/5] Optimizing wasm file
   ... 
   Compiling flipper v0.1.0 (/var/folders/8g/_rkmq65n0qnf70c41xvp174h0000gn/T/cargo-contract_Qv7HEC)
   Compiling metadata-gen v0.1.0 (/var/folders/8g/_rkmq65n0qnf70c41xvp174h0000gn/T/cargo-contract_Qv7HEC/.ink/metadata_gen)
    Finished release [optimized] target(s) in 36.29s
     Running `target/release/metadata-gen`
 [4/5] Generating bundle
 [5/5] Generating metadata
	
Original wasm size: 19.1K, Optimized: 2.5K

Your contract artifacts are ready. You can find them in:
/Users/alexander/Developer/parity/contracts/flipper/target

  - flipper.contract (code + metadata)
  - flipper.wasm (the contract's code)
  - metadata.json (the contract's metadata)

Robbepop

WIP: review - will continue later

Robbepop · 2020-11-09T14:26:36Z

README.md

-    build                Compiles the smart contract
-    generate-metadata    Generate contract metadata artifacts
+    build                Compiles the contract, generates metadata, bundles both together in a '.contract' file
+    check                Check that the code builds as Wasm; does not output any build artifact to the top level `target/` directory


Maybe we should mention that this step also deploys (or will deploy) some custom checks that we implement on top besides just making sure that things compile for Wasm.

I agree, but would only add it once we actually have some checks on top.

Robbepop · 2020-11-09T14:28:17Z

metadata/lib.rs

+    #[serde(skip_serializing_if = "Option::is_none")]
+    hash: Option<CodeHash>,


When is this ever None? I guess it is always useful even if we provide the sources.

Discussion happened above, here.

What I read from the discussion is that we still always want the hash field to be available. And also I don't think that we need --metadata-only. Please convince me of the opposite by coming up with a useful use case.

What I read from the discussion is that we still always want the hash field to be available.

I understood the discussion in the way that we want to omit the hash:

It is not required, currently. It's just there as a nice to have. In the future the client tools could use that (if present) to verify the metadata matches the deployed code.

So I would say that if metadata-only is specified then that field should be omitted, even if the wasm is already there because it is not guaranteed to match.

athei · 2020-11-10T08:50:43Z

It is still saying "Optimizing wasm file" when it actually goes on and starts building "metadata-gen":

alexander:~/Developer/parity/contracts/flipper% cargo contract build
 [1/5] Building cargo project
    Finished release [optimized] target(s) in 0.04s
 [2/5] Post processing wasm file
 [3/5] Optimizing wasm file
    Updating crates.io index
   Compiling metadata-gen v0.1.0 (/var/folders/8g/_rkmq65n0qnf70c41xvp174h0000gn/T/cargo-contract_LmksAM/.ink/metadata_gen)
    Finished release [optimized] target(s) in 1.37s
     Running `target/release/metadata-gen`
 [4/5] Generating metadata
 [5/5] Generating bundle
	
Original wasm size: 19.1K, Optimized: 2.5K

Your contract artifacts are ready. You can find them in:
/Users/alexander/Developer/parity/contracts/flipper/target

  - flipper.contract (code + metadata)
  - flipper.wasm (the contract's code)
  - metadata.json (the contract's metadata)

athei

LGTM

cmichi added 3 commits November 2, 2020 20:41

Fix URIs

5d26731

Make generate-metadata output consistent with build

806929f

Add cargo contract pack

7161a6d

cmichi requested a review from ascjones November 2, 2020 20:02

ascjones reviewed Nov 3, 2020

View reviewed changes

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/cmd/metadata.rs Show resolved Hide resolved

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/cmd/metadata.rs Outdated Show resolved Hide resolved

cmichi added 8 commits November 3, 2020 17:55

Return error instead of panicking

5e3f520

Use blake2_hash()

e7fbaba

Replace match with if

61f7261

Pass reference instead of ownership

6cc0a67

Generate metadata.json and <contract>.pack

d9eb49e

Output .wasm, .json, .pack

4d1edfa

Return result object instead of tuple

0eb1949

Get it to run with '--features test-ci-only'

3b3206e

athei reviewed Nov 4, 2020

View reviewed changes

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

cmichi and others added 3 commits November 4, 2020 14:01

Rename .pack to .contract

f5450d6

Apply suggestions from code review

c71639f

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

Introduce '--skip-packing' and '--skip-metadata'

2aa7c96

Merge branch 'master' into cmichi-implement-cargo-pack

9fc63a0

athei reviewed Nov 5, 2020

View reviewed changes

Robbepop reviewed Nov 5, 2020

View reviewed changes

Apply suggestions from code review

b40a82e

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

Print optimization result at the end

9ed109b

Improve output

693f987

ascjones reviewed Nov 9, 2020

View reviewed changes

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/cmd/build.rs Outdated Show resolved Hide resolved

cmichi added 4 commits November 9, 2020 11:37

Replace 5-tuple return value with struct

160b992

Include hash only for bundle in metadata

c699312

Make it work with --features test-ci-only

923cded

Fix doc test

fcb66c6

ascjones reviewed Nov 9, 2020

View reviewed changes

metadata/lib.rs Outdated Show resolved Hide resolved

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/cmd/metadata.rs Outdated Show resolved Hide resolved

src/cmd/metadata.rs Show resolved Hide resolved

ascjones reviewed Nov 9, 2020

View reviewed changes

src/cmd/metadata.rs Show resolved Hide resolved

cmichi added 3 commits November 9, 2020 14:14

Remove comments

c5296ad

Introduce wrapper type CodeHash

4cdc220

Make it work with --features test-ci-only

930a309

ascjones approved these changes Nov 9, 2020

View reviewed changes

cmichi added 2 commits November 9, 2020 15:30

Display important results bold

4403bd7

Include size diff for code-only as well

0c596f0

Robbepop reviewed Nov 9, 2020

View reviewed changes

cmichi added 5 commits November 9, 2020 19:16

Remove comment

d9a0f10

Shorten code

8737b9b

Clone metadata for correct UI output

234b45f

Remove unnecessary return

8474209

Fix return type

b6b9cad

Print metadata generation in correct step

1c067d9

athei approved these changes Nov 10, 2020

View reviewed changes

ascjones merged commit 144ea27 into master Nov 10, 2020

ascjones deleted the cmichi-implement-cargo-pack branch November 10, 2020 11:38

ascjones mentioned this pull request Nov 27, 2020

Release v0.8.0 #111

Merged

cmichi mentioned this pull request Apr 21, 2021

Remove deprecated generate-metadata command #265

Merged

		#[serde(skip_serializing_if = "Option::is_none")]
		hash: Option<CodeHash>,

Conversation

cmichi commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athei left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ascjones commented Nov 4, 2020

Uh oh!

cmichi commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmichi Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop commented Nov 5, 2020

Uh oh!

athei commented Nov 5, 2020

Uh oh!

Robbepop commented Nov 5, 2020

Uh oh!

athei commented Nov 5, 2020

Uh oh!

Robbepop commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmichi commented Nov 5, 2020

Uh oh!

Robbepop commented Nov 5, 2020

Uh oh!

cmichi commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

cmichi commented Nov 2, 2020 •

edited

Loading

athei left a comment •

edited

Loading

cmichi commented Nov 5, 2020 •

edited

Loading

cmichi Nov 5, 2020 •

edited

Loading

Robbepop Nov 5, 2020 •

edited

Loading

Robbepop Nov 5, 2020 •

edited

Loading

Robbepop commented Nov 5, 2020 •

edited

Loading

cmichi commented Nov 9, 2020 •

edited

Loading