0xPolygonZero / plonky2

Apache License 2.0
777 stars 289 forks source link

restore `no-std` support #1335

Closed muraca closed 1 year ago

muraca commented 1 year ago

no-std support for plonky2 and starky was added by #806, but nobody noticed some breaking changes later on. This PR restores no-std support, and introduces wasm32-unknown-unknown as an additional target for tests, to avoid making the same mistake again.

https://github.com/0xPolygonZero/plonky2/pull/907#issuecomment-1462939316

Nashtare commented 1 year ago

The CI isn't running with your changes: https://github.com/0xPolygonZero/plonky2/actions/runs/6782203615. You may want to try it out locally against your own fork first.

Nashtare commented 1 year ago

In addition, cargo test cannot be run against a no-std target as it also runs plonky2_evm tests which use std.

Nashtare commented 1 year ago

This doesn't seem to be doing what's expected still.

image

If we were running against a wasm32-unknown-unknown target, the current would fail as we didn't deactivate the default features which include std.

I'd recommend you to update the CI until you have a failing job for wasm complaining about std, and only then remove the default features to have it go green.

Nashtare commented 1 year ago

Thanks @muraca! Tiny final nit, the job is called "Test suite" but we do not run the tests against wasm, so a different name would be preferable.

muraca commented 1 year ago

@Nashtare sorry for the mess. Looking back at it, I think it would be better to have separate jobs instead of the matrix, since we only have to perform 2 additional checks. What do you think?

Nashtare commented 1 year ago

@muraca Yeah, having a dedicated job for wasm32 seems better :)

Nashtare commented 1 year ago

Hmm, the current command run in the CI job for wasm, cargo check --manifest-path plonky2/Cargo.toml --target wasm32-unknown-unknown doesn't trigger any error on main, although it should if not deactivating the default-features. I'll need to investigate it a bit more.

Nashtare commented 1 year ago

@muraca Could you add --no-default-features to the no-std check? This is what's currently missing to enforce that we can properly compile in no-std environments (it fails on main without your fixes). Thanks!

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information