feat: add compiled code size tests

[austinabell]

cc @matklad as he suggested adding these before making other changes/refactoring.

The tests compile to wasm using release and any other usual flags and check the output wasm binary size.

The test checks to make sure it's no larger than 10% of what it currently is, to accommodate for external/platform discrepancies. Still feels a bit janky to me to test like this personally, but seems like it might be a reasonable check to have.

[matklad]

If I am not mistaken, this should not be necessary? actions-rs/toolchain@v1 already installs this target, no?

[austinabell]

yeah that's what I assumed, but there might be something internal which installs it for a different toolchain, not sure. Just added that as a workaround and was going to look into it today

[matklad]

obesrvation: this yml sets a horribly outdated toolchain at the start

[austinabell]

yeah this CI is something I would like to improve/rewrite in the future, as it only checks formatting and runs tests, to give some context into why I thought just to add a workaround for now. I can create an issue for this now, because I think it's a standard to at least check clippy as well.

[matklad]

I'd add Command::new("rustup").args(&["target", "add", "wasm32-unknown-unknown"]).status().unwrap() here, so that the test just works in more cases.

[austinabell]

hmm, that seems like a bad implication to update a user's toolchain unknowingly through running a test, maybe if this test was ignored by default?

[matklad]

It seems so, but I don't think that's the case. Like, when you run cargo test, cargo automatically updates the index and downloads crates, and that doesn't cause problems. Similarly, if you run cargo test in a project with rustup-toolchain, rustup automatically downloads the toolchain without asking for confirmation.

Really, cargo build --target xxx should just work. The reason it doesn't is a sad historical incident -- target management was implemented in rustup, while it really belongs to cargo. So this pattern is a work-around for a bug in the toolchain.

Empirically, I've been using rustup commands in CI in various projects (example from rust-analyzer: https://github.com/rust-analyzer/rust-analyzer/blob/master/xtask/src/main.rs#L80-L89), and it never caused problems.

But maybe I am biased -- I am pretty found of various toolchain hacks which streamline workflows :D

[austinabell]

yeah, that's a fair point. I'll make this change

[austinabell]

reverted the change @matklad as there were interactions through rustup which don't play well in the test, locally and on CI. Let me know if you have a strong opinion on this and I can push the commit again and try to find a way around

[matklad]

Don't have strong opinion! I think it wold be useful, but not critical, to understand what those bad interactions are, but, even if we do that, it should be done separately from this PR.

[austinabell]

Don't have strong opinion! I think it wold be useful, but not critical, to understand what those bad interactions are, but, even if we do that, it should be done separately from this PR.

for posterity, the logs locally were:

error: component download failed for rust-std-wasm32-unknown-unknown
error: caused by: could not rename file
test status_message_code_size_check ... FAILED

and CI run: https://github.com/near/near-sdk-rs/actions/runs/832347131

[matklad]

Aha, that's because rustup, unlike cargo, is not safe to use concurrently. The toolchain installaction code needs to be wrapped into std::sync::Once, at which point it's probably too much work.

[austinabell]

Aha, that's because rustup, unlike cargo, is not safe to use concurrently. The toolchain installation code needs to be wrapped into std::sync::Once, at which point it's probably too much work.

That's a good point, I assumed the overlap wouldn't be an issue because once installed the next call would be just a check. I'll explore in a future PR

[matklad]

hey, looks my issue from a while ago about this is seeing some recent progress: rust-lang/rustup#988