-
Notifications
You must be signed in to change notification settings - Fork 683
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
refactor: make conditional compilation based on target instead of fea… #5902
base: develop
Are you sure you want to change the base?
refactor: make conditional compilation based on target instead of fea… #5902
Conversation
919c5d0
to
e744b1e
Compare
This sounds like a good idea. I see that there are other wasm targets that should all be treated the same. |
@obycode Are you recommending that I use |
I like |
ok, change done 👍 |
I understand the motivation for making these target conditionals, but it kind of breaks the way that I think about features and compilation targets. Features really should capture things that alter interfaces or behavior in the output. Targets should just tell the compilation process "build the thing with the features I specify, for a particular architecture." My preference, I think would be something like the following:
Then that's kind of it, right? To build the packages without |
@kantai One could argue that the rusqlite feature wouldn't compile to certain targets (wasm) but it could be implemented some day. On a related note: |
Context
#4791 made the rusqlite dependency optional by introducing the
canonical
cargo feature (stacks_common/canonical
andclarity/canonical
). This is useful to allow compiling theclarity
crate to wasm32-unknown-unknown.It used cargo feature based conditional compilation.
Improvement
This PRs proposes to use target based conditiona compilation instead, which removes the need to introduce extra cargo features (such as
canonical
) because cargo has built-in support for this, usingtarget_arch = "wasm32"
instead offeature = "canonical"
.It makes the intention easier to understand, and dependency management easier.
clarity
can directly depends onstacks_common
(with default feature) and doesn't have to manage the features it requires.The same change was made in Clarinet recently, it heavily relies on conditional compilation for wasm so the benefits are even bigger there.
Why now?
As part #5891, I want the
develop
andmaster
branches to be able to compile to wasm.rusqlite
is one of the package that needs to be excluded from wasm32 compilation but there are other (secp256k).I want to make sure to use the best approach to do it. And cargo feature isn't the right tool for this job.
Alternatives
Instead of targetting
target_arch = "wasm32"
/not(target_arch = "wasm32")
, we could use target_family;any(target_family = "unix", target_family = "windows")
not(target_family = "wasm")
I'm open to suggestions for that