-
Notifications
You must be signed in to change notification settings - Fork 75
Add feature gates #288
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
base: main
Are you sure you want to change the base?
Add feature gates #288
Conversation
can I ask you why you're doing this or looking into noname? |
Hey @mimoo! I am opening this PR because recently I tried to bump the version of dependencies to latest in Sonobe, which has experimental support for DSLs including Noname, but I faced some compilation issues when targeting wasm. Then I realized that Noname provides some functionalities that are useful as a CLI tool, and for this purpose it depends on wasm-unfriendly libraries such as |
@@ -1,3 +1,4 @@ | |||
#[cfg(feature = "server")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just gate the whole server module in src/lib.rs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
/// Field element helpers | ||
/// Unless otherwise stated everything is in little-endian byte order. | ||
pub trait FieldHelpers<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these have moved here https://github.com/o1-labs/proof-systems/blob/c1803e0a480172c193ba67c826110980303a172f/utils/src/field_helpers.rs#L72 so no need to copy it in noname
@@ -1,3 +1,5 @@ | |||
#[cfg(feature = "kimchi")] | |||
mod examples; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels weird to gate this under kimchi, but I guess maybe less painful than add the feature gate within examples?
Cargo.toml
Outdated
|
||
[features] | ||
default = ["cli"] | ||
cli = ["kimchi", "server"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to have a cli feature? Maybe call this full instead?
that makes sense! Thanks so much for the PR then :) I added a few comments, do you think you can address them? If so I think I could merge this |
This PR adds feature gates for
cli
,r1cs
,kimchi
, andserver
. It allows downstream libraries to select desired features instead of enabling all of them.The version of arkworks libraries is also bumped to the latest.