refactor: centralize&improve infrastructure for building test contracts #4160

matklad · 2021-03-24T11:24:22Z

Move test contracts to the dedicated runtime/near-test-contracts folder
Do not commit .wasm files and instead re-build them on the fly
when necessary
Use build.rs rather than build.sh for compilation
Replace lazy_static_include calls with centralized functions

matklad · 2021-03-24T11:26:51Z

Backstory here: I've noticed that our sum_n method doesn't actually sum the n numbers, as LLVM can compute sums of polynomial terms in closed form. I wanted to fix that (as we do use that method for benchmarking), but realized that I would need to build wasm locally and commit it.

matklad · 2021-03-24T11:36:31Z

One thing I am not sure here is whether our python tests are guaranteed to run only after cargo build. Some of them read .wasm directly from the res folder, and, to populate res, building rust code is required.

matklad · 2021-03-24T11:43:42Z

test-utils/testlib/src/runtime_utils.rs

 pub fn default_code_hash() -> CryptoHash {
-    let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-    path.push("../../runtime/near-vm-runner/tests/res/test_contract_rs.wasm");
-    let genesis_wasm = fs::read(path).unwrap();
-    hash(&genesis_wasm)
+    hash(&near_test_contracts::rs_contract())
 }

 lazy_static::lazy_static! {
-    static ref DEFAULT_TEST_CONTRACT_HASH: CryptoHash = hash(&DEFAULT_TEST_CONTRACT);
-}
-
-lazy_static_include::lazy_static_include_bytes! {
-    DEFAULT_TEST_CONTRACT => "../../runtime/near-vm-runner/tests/res/test_contract_rs.wasm"
+    static ref DEFAULT_TEST_CONTRACT_HASH: CryptoHash = hash(near_test_contracts::rs_contract());
 }


This seems to be some pre-existing duplication: default_code_hash and DEFAULT_TEST_CONTRACT compute the same thing. Left as is, as out of scope for the PR.

chefsale

Seems like tests are failing, lets fix that first. 😄

bowenwang1996 · 2021-03-24T13:31:03Z

runtime/near-test-contracts/Cargo.toml

@@ -0,0 +1,10 @@
+[package]
+name = "near-test-contracts"
+version = "0.0.0"


Does 0.0.0 make sense?

It does -- we are not going to publish this crate, so it doesn't have a meaningful semver. So putting a default 0.1.0 would be (a minor and insignificant) lie. 0.0.0 and publish = false explicitly say that we don't care about semver in this case.

bowenwang1996 · 2021-03-24T13:32:17Z

runtime/near-vm-runner/src/tests/rs_contract.rs

 fn test_contract() -> ContractCode {
-    ContractCode::new(TEST_CONTRACT.to_vec(), None)
+    let code = if cfg!(feature = "protocol_feature_alt_bn128") {


This seems off to me. It should be based on whether nightly_protocol is enabled.

Probably! I've tried to keep pre-existing semantics in this PR, I can change this and that in a follow up.

Took a look here, I am not sure how to fix it. This crate doesn't have nightly_protocol feature:

[features] # all vms enabled for tests, but only one default vm, specified by runtime crate default = ["wasmer0_vm", "wasmtime_vm", "wasmer1_vm"] wasmer0_vm = [ "wasmer-runtime" ] wasmtime_vm = [ "wasmtime", "anyhow"] wasmer1_vm = [ "wasmer", "wasmer-types", "wasmer-compiler-singlepass", "wasmer-compiler-cranelift", "wasmer-engine-native", ] lightbeam = ["wasmtime/lightbeam"] no_cpu_compatibility_checks = [] protocol_feature_evm = ["near-primitives/protocol_feature_evm", "near-evm-runner/protocol_feature_evm"] no_cache = [] protocol_feature_alt_bn128 = [ "near-vm-logic/protocol_feature_alt_bn128", "near-primitives/protocol_feature_alt_bn128", "near-vm-errors/protocol_feature_alt_bn128" ]

At this point, I don't know the purpose of nightly_protocol and nightly_protocol_features cargo features and why they are set up that way, so I can't immediately fix that. Learning more about our feature setup is on my todo list, so I might come back to this later.

@matklad nightly_protocol is supposed to be a marker feature near/NEPs#126 (comment)

matklad · 2021-03-24T18:21:43Z

Got this past CI, including the "One thing I am not sure here is whether our python tests are guaranteed to run only after cargo build" bit.

should be ready for review.

* Move test contracts to the dedicated `runtime/near-test-contracts` folder * Do not commit `.wasm` files and instead re-build them on the fly when necessary * Use build.rs rather than build.sh for compilation * Replace lazy_static_include calls with centralized functions

ailisp · 2021-03-25T16:22:46Z

runtime/near-test-contracts/build.rs

+        format!("./res/{}.wasm", output),
+    )?;
+    println!("cargo:rerun-if-changed=./{}/src/lib.rs", dir);
+    println!("cargo:rerun-if-changed=./{}/Cargo.toml", dir);


What are these println?

They tell cargo when it needs to rebuild the test contracts, These are documented in https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script

In theory, we maybe can just always run cargo build here, as that should be a no-op, but it wasn't hard to save an extra process invocation here.

They were extracted out of near-vm-runner in near#4160 but I forgot to update codeowners.

* add myself as a codeowner * fixup #4160 to re-install codeowners for the new crate.

matklad requested a review from chefsale March 24, 2021 11:24

matklad requested review from ailisp, bowenwang1996, evgenykuzyakov, olonho and SkidanovAlex as code owners March 24, 2021 11:24

matklad force-pushed the near-test-contracts branch 4 times, most recently from 61400d1 to 584b594 Compare March 24, 2021 11:33

matklad force-pushed the near-test-contracts branch from 584b594 to d17ac80 Compare March 24, 2021 11:40

matklad commented Mar 24, 2021

View reviewed changes

chefsale suggested changes Mar 24, 2021

View reviewed changes

matklad force-pushed the near-test-contracts branch 2 times, most recently from 4d414c0 to 023a71d Compare March 24, 2021 12:50

bowenwang1996 reviewed Mar 24, 2021

View reviewed changes

matklad force-pushed the near-test-contracts branch 7 times, most recently from 247b8a0 to 6420c4a Compare March 24, 2021 15:58

matklad force-pushed the near-test-contracts branch from 6420c4a to 08f7cb6 Compare March 25, 2021 12:31

ailisp reviewed Mar 25, 2021

View reviewed changes

ailisp approved these changes Mar 25, 2021

View reviewed changes

bowenwang1996 approved these changes Mar 25, 2021

View reviewed changes

chefsale approved these changes Mar 26, 2021

View reviewed changes

chefsale and others added 2 commits March 26, 2021 00:28

Merge branch 'master' into near-test-contracts

2119dcc

Merge branch 'master' into near-test-contracts

89cc986

matklad added the automerge label Mar 26, 2021

near-bulldozer bot merged commit a767e46 into near:master Mar 26, 2021

mikhailOK mentioned this pull request Mar 26, 2021

Test contracts nayduck failures #4174

Closed

matklad deleted the near-test-contracts branch April 13, 2021 18:16

matklad added a commit to matklad/nearcore that referenced this pull request Apr 13, 2021

Fix code ownership for test contract

7f4c4c5

They were extracted out of near-vm-runner in near#4160 but I forgot to update codeowners.

matklad mentioned this pull request Apr 13, 2021

Update contract runtime code owners #4219

Merged

near-bulldozer bot pushed a commit that referenced this pull request Apr 14, 2021

Update contract runtime code owners (#4219)

0254511

* add myself as a codeowner * fixup #4160 to re-install codeowners for the new crate.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: centralize&improve infrastructure for building test contracts #4160

refactor: centralize&improve infrastructure for building test contracts #4160

matklad commented Mar 24, 2021

matklad commented Mar 24, 2021

matklad commented Mar 24, 2021

matklad Mar 24, 2021

chefsale left a comment •

edited

Loading

bowenwang1996 Mar 24, 2021

matklad Mar 24, 2021

bowenwang1996 Mar 24, 2021

matklad Mar 24, 2021 •

edited

Loading

matklad Apr 7, 2021

bowenwang1996 Apr 8, 2021

matklad commented Mar 24, 2021 •

edited

Loading

ailisp Mar 25, 2021

matklad Mar 25, 2021

refactor: centralize&improve infrastructure for building test contracts #4160

refactor: centralize&improve infrastructure for building test contracts #4160

Conversation

matklad commented Mar 24, 2021

matklad commented Mar 24, 2021

matklad commented Mar 24, 2021

Choose a reason for hiding this comment

chefsale left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefsale left a comment •

edited

Loading

matklad Mar 24, 2021 •

edited

Loading

matklad commented Mar 24, 2021 •

edited

Loading