Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a way to automatically compile and deploy a contract #77

Merged
merged 19 commits into from
Mar 30, 2022

Conversation

itegulov
Copy link
Contributor

Fixes #54

This is a fairly naive implementation that I have tested on all examples from near-sdk-rs. So it should still suit the majority of users, but I can see the following paths for further improvement:

  • Make dev_deploy_project more flexible by introducing some sort of builder where a user would be able to pass custom cargo arguments (or even switch between debug and release as I am just using release by default for everything), env variables, target wasm filename if it is ambiguous (I am not sure if a single project can generate >1 wasm files, but it probably can) etc
  • Introduce a build cache with a mutex for each unique project path. Build cache would save the generated .wasm file to /tmp and all other tests will use it instead of trying to re-compile the project. This will be especially useful for people who have long-running tests that want to edit their codebase while the tests are running.
  • Not sure if this is possible/good idea, but maybe a macro that would do contract compilation at test compile time and save the wasm contents directly to the test binary?
  • Low priority, but would be cool to set up integration tests that would try to compile & deploy some well-known NEAR contracts (kind of like compiler devs do it with third-party libraries to prevent regression)

@itegulov itegulov force-pushed the daniyar/autocompilation branch from a4cbc39 to 2e06d67 Compare February 21, 2022 03:44
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

Overall looks good. Some stuff that might be relevant to model off of since dtolnay has already done custom cargo commands: https://github.com/dtolnay/trybuild/blob/master/src/cargo.rs

I am just using release by default for everything), env variables, target wasm filename if it is ambiguous...

release is fine since for the most part, the sdk is heavily optimized, and we'd want to test for the release binary anyways.

(I am not sure if a single project can generate >1 wasm files, but it probably can) etc

It can if we end up specifying the path to a cargo-workspace project which specifies a bunch of workspace members. But yeah, we can leave behind compiling multiple contracts and lets just force users to specify the specific contract path instead then. This would makes things a lot simpler.

Introduce a build cache with a mutex for each unique project path. Build cache would save the generated .wasm file to /tmp and all other tests will use it instead of trying to re-compile the project. This will be especially useful for people who have long-running tests that want to edit their codebase while the tests are running.

hmm, since we're running the cargo command, cargo would lock further compilation on the same project wouldn't it? Like say test1 already compiles, test2 tries to compiles but there's no changes so cargo wouldn't be triggered at all. Similarly, if they weren't sequential tests, there's a Cargo lockfile that blocks test2 from running cargo. That's what I think happens at least, so might not need need this build cache

would be cool to set up integration tests that would try to compile & deploy some well-known NEAR contracts (kind of like compiler devs do it with third-party libraries to prevent regression)

would definitely like to have some more well known contracts tested. We have a ref-finance contract in the examples, but we don't run github actions on examples as of now

workspaces/src/network/mod.rs Outdated Show resolved Hide resolved
workspaces/src/cargo/mod.rs Outdated Show resolved Hide resolved
workspaces/src/cargo/mod.rs Outdated Show resolved Hide resolved
@itegulov
Copy link
Contributor Author

@ChaoticTempest

Some stuff that might be relevant to model off of since dtolnay has already done custom cargo commands: https://github.com/dtolnay/trybuild/blob/master/src/cargo.rs

Thanks for the pointer, I will take a look

release is fine since for the most part, the sdk is heavily optimized, and we'd want to test for the release binary anyways.

Yeah, that is why I chose it to be the default. I was just thinking that there could be some use cases where a user would want to use custom cargo commands that I am not aware of, but I guess we can gather some feedback from users before stabilizing this feature. WDYT in general about making this an unstable feature for now?

hmm, since we're running the cargo command, cargo would lock further compilation on the same project wouldn't it? Like say test1 already compiles, test2 tries to compiles but there's no changes so cargo wouldn't be triggered at all. Similarly, if they weren't sequential tests, there's a Cargo lockfile that blocks test2 from running cargo. That's what I think happens at least, so might not need need this build cache

I think you misunderstood my point here. Since we are trying to compile codebase located on the user's machine it can change at any time during the test execution. Let's say I am running 16 long-running tests in parallel using 8 threads. As you said, test 1 will successfully build the cargo project and tests 2-8 will try to build but since there will be nothing to do and they will just reuse the existing compilation artifact. But each of these tests requires 20 minutes to finish, in the meantime an impatient developer starts making changes to the contract codebase. Tests 1-8 finish and now it's turn for tests 9-16; they invoke cargo build but it fails because the project is already in an incompilable state because the user was in the middle of writing a new function. Or, even worse, it was in a compilable state but tests 9-16 used a different wasm file that has a freshly introduced bug. Imagine how confusing it would be 😅

@ChaoticTempest
Copy link
Member

WDYT in general about making this an unstable feature for now?

That's a good idea. It's definitely more of something that could help us, but the experience just isn't fully hammered out. Let's leave it for experimental reasons then

because the user was in the middle of writing a new function

Gotcha. Yeah then all the more reason to make this feature unstable. It would definitely be unintuitive, but if its under a feature flag, we can educate users on the caveats of this feature if we don't immediately go the build cache route

workspaces/src/cargo/mod.rs Show resolved Hide resolved
workspaces/src/network/mod.rs Outdated Show resolved Hide resolved
workspaces/src/network/mod.rs Outdated Show resolved Hide resolved
workspaces/tests/test-contracts/ft/Cargo.toml Outdated Show resolved Hide resolved
workspaces/tests/test-contracts/ft/src/lib.rs Outdated Show resolved Hide resolved
@itegulov itegulov force-pushed the daniyar/autocompilation branch from 8bf776d to fc42849 Compare February 28, 2022 06:39
@@ -0,0 +1,26 @@
#![recursion_limit = "256"]
Copy link
Member

Choose a reason for hiding this comment

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

We'll also want #![cfg(feature = "unstable")] on top since cargo test will try to run it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I have also made so that GHA runs tests with --features unstable since that is what we are doing in near-sdk-rs, but lmk if you want me to create two separate workflows for stable/unstable version

workspaces/src/cargo/mod.rs Outdated Show resolved Hide resolved
@itegulov
Copy link
Contributor Author

itegulov commented Mar 4, 2022

@ChaoticTempest

Some stuff that might be relevant to model off of since dtolnay has already done custom cargo commands: https://github.com/dtolnay/trybuild/blob/master/src/cargo.rs

I have pulled a configurable cargo binary from there. Also tried out offline mode, but I misunderstood how it works and despite it claiming that it avoids making network calls it actually outright refuses to download dependencies if you don't already have them which isn't good.

workspaces/src/cargo/mod.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@@ -10,9 +10,11 @@ Library for automating workflows and testing NEAR smart contracts.

[dependencies]
async-trait = "0.1"
async-process = "1.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

might be a good idea to make this optional under the unstable flag as well, but not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! Yeah, it probably should be optional and pulled in with this feature

let _res = contract
.call(&worker, "set_status")
.args_json(("foo",))?
.gas(300_000_000_000_000)
Copy link
Member

Choose a reason for hiding this comment

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

could replace this to .max_gas() now that #94 has made it in

@itegulov itegulov merged commit 4c4c4a0 into main Mar 30, 2022
@itegulov itegulov deleted the daniyar/autocompilation branch March 30, 2022 03:44
@frol frol mentioned this pull request Oct 4, 2023
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.

Figure out an easy way to both compile contracts and run them with workspaces
4 participants