refactor: make conditional compilation based on target instead of fea…

[hugocaillard]

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 of #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

[obycode]

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]

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

[obycode]

I like not(target_family = "wasm").

[hugocaillard]

ok, change done 👍

[kantai]

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]

@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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.96%. Comparing base (15eec8d) to head (5be97d3).
Report is 13 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5902      +/-   ##
===========================================
+ Coverage    83.78%   83.96%   +0.17%     
===========================================
  Files          516      516              
  Lines       381284   381279       -5     
===========================================
+ Hits        319467   320127     +660     
+ Misses       61817    61152     -665     

Files with missing lines	Coverage Δ
clarity/src/vm/analysis/mod.rs	95.23% <ø> (ø)
clarity/src/vm/database/clarity_store.rs	40.00% <ø> (ø)
clarity/src/vm/docs/contracts.rs	95.52% <ø> (ø)
clarity/src/vm/errors.rs	75.30% <ø> (ø)
clarity/src/vm/mod.rs	90.61% <ø> (ø)
stacks-common/src/bitvec.rs	91.92% <ø> (ø)
stacks-common/src/types/mod.rs	92.10% <ø> (ø)
stacks-common/src/util/macros.rs	85.71% <ø> (ø)

... and 47 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3123ccb...5be97d3. Read the comment docs.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[obycode]

LGTM