Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

hugocaillard
Copy link
Collaborator

Context

#4791 made the rusqlite dependency optional by introducing the canonical cargo feature (stacks_common/canonical and clarity/canonical). This is useful to allow compiling the clarity 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, using target_arch = "wasm32" instead of feature = "canonical".
It makes the intention easier to understand, and dependency management easier. clarity can directly depends on stacks_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 and master 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")
  • or not(target_family = "wasm")

I'm open to suggestions for that

@hugocaillard hugocaillard requested review from a team as code owners March 6, 2025 13:26
@hugocaillard hugocaillard requested a review from obycode March 6, 2025 13:26
@hugocaillard hugocaillard force-pushed the refactor/wasm-target-conditional-compilation branch from 919c5d0 to e744b1e Compare March 6, 2025 13:27
@obycode
Copy link
Contributor

obycode commented Mar 6, 2025

Instead of targetting target_arch = "wasm32" / not(target_arch = "wasm32"), we could use target_family

This sounds like a good idea. I see that there are other wasm targets that should all be treated the same.

@hugocaillard
Copy link
Collaborator Author

@obycode Are you recommending that I use any(target_family = "unix", target_family = "windows") instead?

@obycode
Copy link
Contributor

obycode commented Mar 6, 2025

I like not(target_family = "wasm").

@hugocaillard
Copy link
Collaborator Author

ok, change done 👍

@kantai
Copy link
Member

kantai commented Mar 6, 2025

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:

  1. Almost all the places in the code that currently use cfg(feature = "canonical") should use cfg(feature = "rusqlite"). The rusqlite feature should control whether or not libclarity and stacks-common are built with support for rusqlite types.
  2. rusqlite shouldn't be a default feature! This is maybe a controversial suggestion, but I think it makes sense here. The packages in this repo (stacks-core) which require rusqlite support in libclarity and stacks-common can easily specify that in their Cargo.tomls. Our testing infrastructure can and already should run with --all-features.

Then that's kind of it, right? To build the packages without rusqlite, you just don't supply the feature.

@hugocaillard
Copy link
Collaborator Author

@kantai
Ok that makes sense. Especially if there's a case to build clarity for unix|windows without rusqlite.
This PR could just be renaming the canonical cargo feature to rusqlite, it would already be any improvement. Or could we give it a more meaninful name like memory_backing_store? I don't have a broad enough knowledge of this repo to know if that makes sense

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:
I'll do something similar for secp256k1 (that doesn't compile to wasm), but in the case, using target_family will make a lot of sense since it will build the same feature set (but with a different dependency). This is how it's handled currently on the clarity-wasm branch – using a feature named wasm was a mistake here

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