-
Notifications
You must be signed in to change notification settings - Fork 55
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
Dependency mess should be fixed #146
Comments
As mentioned in #146 there's a big issue that involves some dependencies of the crate. As a temporary fix, this forces the workspace to rely on a "non-existing" version of `ark-grumpkin` which is immediately patched at workspace-level for a custom version that @arnaucube owns with some cherry-picked commits. While this allows the CI to pass and crate to build, a better solution is needed.
* fix: Use `target_pointer_size` conditional compilation There are some parts of the code where is needed to de/serialze `usize`s. These, have sizes that vary depending on the target achitecture the code is compiled for. Hence, this adapts the de/serialization to the specific pointer size for which the crate is being compiled to. * change: Support WASM-compatibility and polish Cargo.toml In order to support Wasm-compat and to simplify and improve `Cargo.toml` readability, the follwing changes have been made: - All the deps that can use `parallel` feature, do so. As `rayon` supports non-threaded targets with a fallback option. See: https://docs.rs/rayon-core/1.12.1/rayon_core/index.html#global-fallback-when-threading-is-unsupported - `ark-grumpking` has been brought to `0.5.0-alpha.0` as `0.4.0` appears to not be in `crates.io` anymore. See: https://crates.io/crates/ark-grumpkin/versions - By default, the crate uses `"ark-circom/default"` which selects the `wasmer/sys` feature such that it knows where wasmer is suposed to be run`. - Added a `wasm` feature which forces `ark-circom/wasm` to be used instead. Which internally selects the `wasmer/js` backend to be used such that in-browser execution is possible. - Added `getrandom` with `js` feature as dependency when `wasm32-unknown-unknown` target is selected such that compilation of the crate for testing or simply building is possible. Notice that with `wasi` and other wasm targets, this is not the case as they're automatically supported. For more info, please check: https://docs.rs/getrandom/latest/getrandom/#webassembly-support * feat: Support WASM-compatibility tests in CI Add support for both testing the build of `sonobe/folding-schemes` for WASM-targets and also, it's build as a dependency for a WASM-crate. This includes a build job for the three main supported rust-WASM targets and the same but for a thrid crate creted on-the-fly which uses `sonobe/folding-schemes` as a dependency. * chore: Add README docs about WASM-compat & feats * ci: don't run WASM-compat job if PR is draft * chore: depend on `arnaucube/circom-compat` fork. Since arnaucube/circom-compat#2 was merged, we can already switch to it as we were depending before. * chore: minimal build/test instructions * fix: CI typos * fix: Update CI to use correct feature sets * fix: `ark-grumpkin` versioning issues As mentioned in #146 there's a big issue that involves some dependencies of the crate. As a temporary fix, this forces the workspace to rely on a "non-existing" version of `ark-grumpkin` which is immediately patched at workspace-level for a custom version that @arnaucube owns with some cherry-picked commits. While this allows the CI to pass and crate to build, a better solution is needed. * fix: Clippy CI avoiding --all-targets * fix: use `wasm` feat only with folding-schemes
I noticed that the Git dependencies are currently pinned to a branch name. For better stability and reproducibility, would it be possible to switch to using a specific commit rev instead of a branch? This way, the dependency won't unintentionally change if new commits are pushed to the branch. |
Agree. As commented in chat, the curves patches exist because when we started using the Grumpkin curve implementation there was no v0.5.0 of arkworks and Grumpkin was not included in v0.4.0, so we needed a way to use the last arkworks version while on a stable interface (ie. v0.4.0) but including also the Grumpkin curve, hence the patched fork. We can do a 1-week coordinated effort this September (once current various implementations that are being done are completed to avoid more gitconflicts) where we coordinately upgrade all the dependencies and also Sonobe to arkworks v0.5.0 (to avoid different repos using different versions), cleaning part of the dependencies patches on the way, and we can use the same effort to try to minimize the other dependencies as you mention. Once we do that we can also change to specify the specific commit rev instead of branch in the cases that we still import a custom fork (ideally we would reduce the number of custom fork imports). Apart from the coordinated effort across multiple repos, it would be also across current branches being implemented to minimize git conflicts. |
Currently as I worked on #142 I realized how complex the dependency graph of this workspace is.
And it's not that the workspace is crazily complex. But rather that we have:
Lots of forks across different users and repos:
Some dependencies that also fall under other users:
While I don't have any issues with repos being under the users, it makes it a nightmare to actually update dependencies across the entire workspace. As always some deps have again all the arkworks stack in a older version and it's the never-ending story.
We rely on non-existing versions of crates to make all this juggling to work
If you notice, in all the
Cargo.toml
files we depend onark-grumpkin
, we depend on it with version0.4.0
. But if you actually look at https://crates.io/crates/ark-grumpkin/versions, you'll realize that onlyark-grumpkin v0.5.0-alpha.0
exists published. Therefore, we only use an "imaginary version" such that we can then patch it at top-level workspaceCargo.toml
with a custom version that @arnaucube owns.A lot of the arkworks cherry-picks that we currently patch have already been released.
For most of the arkworks backend,
0.5.0-alpha.0
is already out. This means most, to not say all of the things we have patched should disappear or minimize as much as possible.Action items:
v0.5.0-alpha.0
.v0.5.0-alpha.0
.The text was updated successfully, but these errors were encountered: