Skip to content

No std support #3297

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

Closed
wants to merge 1 commit into from
Closed

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Jun 2, 2023

This PR aims to make as much as the cairo-lang libs no_std compatible.

In order to keep coherence across the crates, I opted for the cairo-lang-std approach.
I added a crate that will export a custom "std" lib, either from the rust "std" one or from the "core" one (+ hashbrown).

This approach reduces the amount of conditional compilation (#[cfg(feature = "std")] vs #[cfg(not(feature = "std"))] ) to be expressed in the subsequent crates.

The drawback is that we have to import all our basics from this cairo-lang-std crate, rather than using core. Regardless, I think that it's the best option for such a diverse workspace.

I also added an ensure_no_std crate. It imports the different libs that support no-std and compiles, with them as dependencies. If even a single symbol across the dependencies is provided by the std lib, the compilation will fail. It is to be used in the CI in order to make sure no regressions are introduced.
It has to be compile with:

cargo +nightly build --target wasm32-unknown-unknown

meaning it cannot be part of the workspace. I added a .cargo/config.toml and a rust-toolchain.toml so running cargo build will be enough.

In it's current state cairo-lang-std, cairo-lang-utils and cairo-lang-casm are compilable in no-std.


This change is Reviewable

@tdelabro tdelabro force-pushed the no_std-support branch 2 times, most recently from bf40b17 to ff9a60e Compare June 6, 2023 09:47
Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 42 files reviewed, 1 unresolved discussion (waiting on @tdelabro)


crates/cairo-lang-project/Cargo.toml line 11 at r2 (raw file):

[dependencies]
cairo-lang-filesystem = { path = "../cairo-lang-filesystem", version = "1.1.0" }
cairo-lang-utils = { path = "../cairo-lang-utils", default-features = false }

Why did u remove the version?

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 42 files reviewed, 3 unresolved discussions (waiting on @tdelabro)


Cargo.toml line 73 at r2 (raw file):

id-arena = "2.2.1"
ignore = "0.4.20"
indexmap = { git = "https://github.com/bluss/indexmap.git", rev = "ca5f848e10c31e80aeaad0720d14aa2f6dd6cfb1", features = [

Depending on git commits is probelamtci, and messes with resolution a lot of times. Why isn't a version enough?


Cargo.toml line 77 at r2 (raw file):

], default-features = false }
indoc = { version = "2.0.1", default-features = false }
itertools = { version = "0.10.5", default-features = false, features = [

Format this better?

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 6, 2023

Why did you remove the version?

This answer describes what happens if you combine path and version. If you are importing the same version as the workspace version, it is not useful at all. And it could lead to errors if not updated. When I believe we always want to use the file from our current workspace commit.

Depending on git commits is probelamtci, and messes with resolution a lot of times. Why isn't a version enough?

indexmap has a problem with no-std in its 1.X.X version. It is described here. So we have to use code that is on version 2.0.0-pre, it could be achieved using version or the branch main. But they are not respecting semantic versioning yet for this version (because it's still -pre). So they could include breaking changes over time, that is why I preferred to fix on a commit that does what I need and is no-std compatible until they release a fixed 2.0.0 tag.

Format this better?

Yes my toml formating tool does this. Which is not the case of most people. I have to change it

@mkaput
Copy link
Member

mkaput commented Jun 6, 2023

indexmap has a problem with no-std in its 1.X.X version. It is described here. So we have to use code that is on version 2.0.0-pre, it could be achieved using version or the branch main. But they are not respecting semantic versioning yet for this version (because it's still -pre). So they could include breaking changes over time, that is why I preferred to fix on a commit that does what I need and is no-std compatible until they release a fixed 2.0.0 tag.

The issue is that using git-based dependencies makes the compiler unpublishable to crates.io which many projects depend on :(

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 6, 2023

The issue is that using git-based dependencies makes the compiler unpublishable to crates.io which many projects depend on :(

The only solution I see is for indexmap to publish a tag 2.0.0-pre. I will try to talk with them and understand what is the feasibility of this.

@tdelabro tdelabro force-pushed the no_std-support branch 4 times, most recently from 508d15b to 2d5c401 Compare June 19, 2023 21:23
@tdelabro tdelabro marked this pull request as ready for review June 19, 2023 21:24
@tdelabro tdelabro marked this pull request as draft June 19, 2023 21:24
@tdelabro tdelabro marked this pull request as ready for review June 24, 2023 09:42
@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 24, 2023

@mkaput On my demand indexmap released the 2.0.0 version

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

@tdelabro great! Then this unblocks this PR from my perspective :)

Reviewable status: 0 of 59 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware and @tdelabro)

@tdelabro
Copy link
Contributor Author

@spapinistarkware wdyt?

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 57 files at r3, all commit messages.
Reviewable status: 5 of 59 files reviewed, 4 unresolved discussions (waiting on @tdelabro)


crates/cairo-lang-casm-contract-class/Cargo.toml line 22 at r4 (raw file):

    "num-bigint/std",
    "serde/std",
]

Add newlines at the end of files

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 59 files reviewed, 5 unresolved discussions (waiting on @tdelabro)


crates/cairo-lang-casm-contract-class/Cargo.toml line 2 at r4 (raw file):

[package]
name = "cairo-lang-casm-contract-class"

I wonder if this is the right name for this package.
We might want to add things in the future here, right? Ant iot won't be just for the contract class

@tdelabro
Copy link
Contributor Author

@spapinistarkware Yes I wasn't sure about the naming, but I don't know what will happen in the future for those crates.
We could always deprepate thoses and create new ones, with the correct name for their content.
Or do you prefer a more vague crate name, like cairo-lang-casm-types maybe ?

@tdelabro tdelabro force-pushed the no_std-support branch 2 times, most recently from e8b57b6 to f619d88 Compare July 3, 2023 12:42
add ensure_no_std crate

move Felt252 as_short_string fn to cairo-lang-utils

remove unused deps from cario-lang-runner

add crate cairo-lang-vm-utils

add crate cairo-lang-casm-contract-class

upgrade cairo-rs dependencies to 0.6.0

build: bump indexmap to 2.0.0

format: ran cargo fmt
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.

3 participants