`[ink_e2e]` utilize `contract-build` crate #1523

ascjones · 2022-11-29T10:27:16Z

use-ink/cargo-contract#787 introduces the new contract-build library, which we can utilize here instead of depending on a user installed version of cargo-contract.

codecov-commenter · 2022-11-30T10:52:20Z

Codecov Report

Merging #1523 (937cf24) into master (40e872a) will increase coverage by 6.61%.
The diff coverage is 0.00%.

❗ Current head 937cf24 differs from pull request most recent head 6580c3b. Consider uploading reports for the commit 6580c3b to get more accurate results

@@            Coverage Diff             @@
##           master    #1523      +/-   ##
==========================================
+ Coverage   65.08%   71.69%   +6.61%     
==========================================
  Files         204      204              
  Lines        6315     6321       +6     
==========================================
+ Hits         4110     4532     +422     
+ Misses       2205     1789     -416

Impacted Files	Coverage Δ
crates/e2e/macro/src/codegen.rs	`0.00% <0.00%> (ø)`
crates/allocator/src/bump.rs	`86.77% <0.00%> (-3.14%)`	⬇️
crates/ink/codegen/src/generator/dispatch.rs	`94.19% <0.00%> (-0.43%)`	⬇️
crates/metadata/src/layout/mod.rs	`77.50% <0.00%> (+1.66%)`	⬆️
crates/ink/ir/src/ir/attrs.rs	`82.27% <0.00%> (+3.60%)`	⬆️
crates/ink/ir/src/ir/item_impl/callable.rs	`91.91% <0.00%> (+4.04%)`	⬆️
crates/ink/ir/src/ir/trait_def/item/mod.rs	`88.61% <0.00%> (+4.87%)`	⬆️
crates/ink/ir/src/ir/selector.rs	`87.75% <0.00%> (+8.16%)`	⬆️
crates/ink/ir/src/ir/idents_lint.rs	`71.42% <0.00%> (+9.52%)`	⬆️
crates/storage/traits/src/layout/impls.rs	`10.00% <0.00%> (+10.00%)`	⬆️
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

HCastano

I see you had a bigger vision in mind already with use-ink/cargo-contract#787 👌

I'd approve, but I haven't been able to build this locally on my Mac or using Docker wit the latest production CI image. I keep running into the following issue

error: custom attribute panicked
  --> lib.rs:57:9
   |
57 |         #[ink_e2e::test]
   |         ^^^^^^^^^^^^^^^^
   |
   = help: message: contract build for Cargo.toml failed: Mismatching versions of `parity-scale-codec` were found!
           Please ensure that your contract and your ink! dependencies use a compatible version of this package.

error: custom attribute panicked
   --> lib.rs:109:9
    |
109 |         #[ink_e2e::test]
    |         ^^^^^^^^^^^^^^^^
    |
    = help: message: Once instance has previously been poisoned

CI passes and local output from cargo-tree looks okay, so not sure what's up, but worth looking into before we merge this

HCastano · 2022-12-01T17:06:20Z

crates/e2e/macro/src/codegen.rs

+    let manifest_path =
+        ManifestPath::new(path_to_cargo_toml).expect("Invalid manifest path");


We should mention what path we tried to use

HCastano · 2022-12-01T17:08:38Z

crates/e2e/macro/src/codegen.rs

+        optimization_passes: Default::default(),
+        keep_debug_symbols: false,
+        lint: false,
+        output_type: Default::default(),


This was OutputType::Json before, I guess changing this is fine now since we don't manually need to parse the build output, right?

Correct 👍

Based on this observation I've made all options explicit instead of relying on Default::default() which could yield some invisible breaking changes, e.g. I recently changed OptimizationPasses to an Option.

ascjones · 2022-12-02T09:46:23Z

I see you had a bigger vision in mind already with paritytech/cargo-contract#787 ok_hand

I'd approve, but I haven't been able to build this locally on my Mac or using Docker wit the latest production CI image. I keep running into the following issue
error: custom attribute panicked
  --> lib.rs:57:9
   |
57 |         #[ink_e2e::test]
   |         ^^^^^^^^^^^^^^^^
   |
   = help: message: contract build for Cargo.toml failed: Mismatching versions of `parity-scale-codec` were found!
           Please ensure that your contract and your ink! dependencies use a compatible version of this package.

error: custom attribute panicked
   --> lib.rs:109:9
    |
109 |         #[ink_e2e::test]
    |         ^^^^^^^^^^^^^^^^
    |
    = help: message: Once instance has previously been poisoned
CI passes and local output from cargo-tree looks okay, so not sure what's up, but worth looking into before we merge this

It's a misleading error, it is in fact because of using cargo +nightly test. See this issue use-ink/cargo-contract#746.

The difference now is that previously we invoked cargo +stable contract, now it uses the lib the cargo invocation inherits the +nightly so we get this error.

So the question is how to handle this. We could:

Force stable toolchain on underlying contract build, even if the parent cargo invocation has a different toolchain
Inherit the parent toolchain but throw a proper error ("must use stable" instead of "mismatching dependencies")
Inherit the parent toolchain and allow to build with nightly (just check the minimum version)

3. is the most cargo like way to do it, but I can't remember exactly why we are enforcing using the stable toolchain.

Update: I've done 3. in use-ink/cargo-contract#848. So we can either wait for that to be merged/released, or merge this one as is and accept that it doesn't allow cargo +nightly test for now.

HCastano · 2022-12-05T20:22:10Z

Sidenote, I remember why I was running the E2E tests with nightly. You can't do cargo contract test --features e2e-tests since the --features flag isn't supported by cargo-contract. I opted to run with cargo +nightly test instead (although cargo +stable test also would've worked)

HCastano

Nice!

HCastano · 2022-12-05T20:28:31Z

crates/e2e/macro/Cargo.toml

@@ -21,10 +21,11 @@ proc-macro = true

 [dependencies]
 ink_ir = { version = "4.0.0-beta", path = "../../ink/ir" }
+contract-build = "2.0.0-beta"


So we can either pull in a Git dep here so that we can use +nightly again, or wait until a new version of cargo-contract is released. I'm okay with waiting for a new version of cargo-contract

Updated to the latest 2.0.0-beta.1 release which allows +nightly. Also --features can be passed through now anyway from the command line so you can also do cargo contract test --features e2e-tests

ascjones · 2022-12-06T09:18:45Z

Sidenote, I remember why I was running the E2E tests with nightly. You can't do cargo contract test --features e2e-tests since the --features flag isn't supported by cargo-contract. I opted to run with cargo +nightly test instead (although cargo +stable test also would've worked)

We should definitely pass the features through then (update see use-ink/cargo-contract#853). For me cargo test --features e2e-tests works though (without nightly), curious what requires the nightly features in this case.

ascjones added 4 commits November 29, 2022 10:16

E2E: utilize contract-build crate

148f43f

Merge branch 'master' into aj/contract-build-lib

50e88c1

Build as debug so we can see the debug messages

52f2024

Merge branch 'master' into aj/contract-build-lib

21c37f5

ascjones changed the title ~~E2E: utilize contract-build crate~~ [ink_e2e] utilize contract-build crate Dec 1, 2022

Switch to using contract-build master branch

963cfee

ascjones marked this pull request as ready for review December 1, 2022 12:52

ascjones requested review from cmichi and HCastano as code owners December 1, 2022 12:52

ascjones added 2 commits December 1, 2022 12:54

Add missing flag

e9701ea

Use released contract-build crate

cdbcbe0

HCastano reviewed Dec 1, 2022

View reviewed changes

ascjones added 3 commits December 1, 2022 17:28

Manifest path error detail

ba97fb2

Be explicit with ExecuteArgs instead of using defaults

2499a4b

Fix warning

937cf24

ascjones mentioned this pull request Dec 2, 2022

Remove rust toolchain channel check use-ink/cargo-contract#848

Merged

HCastano approved these changes Dec 5, 2022

View reviewed changes

ascjones added 2 commits December 7, 2022 10:03

Use latest cargo-contract crates

4167543

Pass features

6580c3b

ascjones merged commit bf2de8f into master Dec 7, 2022

ascjones deleted the aj/contract-build-lib branch December 7, 2022 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`[ink_e2e]` utilize `contract-build` crate #1523

`[ink_e2e]` utilize `contract-build` crate #1523

ascjones commented Nov 29, 2022 •

edited

Loading

codecov-commenter commented Nov 30, 2022 •

edited

Loading

HCastano left a comment

HCastano Dec 1, 2022

HCastano Dec 1, 2022

ascjones Dec 1, 2022

ascjones Dec 1, 2022

ascjones commented Dec 2, 2022 •

edited

Loading

HCastano commented Dec 5, 2022

HCastano left a comment

HCastano Dec 5, 2022

ascjones Dec 7, 2022

ascjones commented Dec 6, 2022 •

edited

Loading

		let manifest_path =
		ManifestPath::new(path_to_cargo_toml).expect("Invalid manifest path");

[ink_e2e] utilize contract-build crate #1523

[ink_e2e] utilize contract-build crate #1523

Conversation

ascjones commented Nov 29, 2022 • edited Loading

codecov-commenter commented Nov 30, 2022 • edited Loading

Codecov Report

HCastano left a comment

Choose a reason for hiding this comment

HCastano Dec 1, 2022

Choose a reason for hiding this comment

HCastano Dec 1, 2022

Choose a reason for hiding this comment

ascjones Dec 1, 2022

Choose a reason for hiding this comment

ascjones Dec 1, 2022

Choose a reason for hiding this comment

ascjones commented Dec 2, 2022 • edited Loading

HCastano commented Dec 5, 2022

HCastano left a comment

Choose a reason for hiding this comment

HCastano Dec 5, 2022

Choose a reason for hiding this comment

ascjones Dec 7, 2022

Choose a reason for hiding this comment

ascjones commented Dec 6, 2022 • edited Loading

`[ink_e2e]` utilize `contract-build` crate #1523

`[ink_e2e]` utilize `contract-build` crate #1523

ascjones commented Nov 29, 2022 •

edited

Loading

codecov-commenter commented Nov 30, 2022 •

edited

Loading

ascjones commented Dec 2, 2022 •

edited

Loading

ascjones commented Dec 6, 2022 •

edited

Loading