spl-collections: fix borsh builds for string wrappers

[grod220]

from_utf8 is a function that was only imported behind the wincode feature. As a result, downstream crates using spl-collections with features = ["borsh"] failed to compile.

[febo]

Looks great – thanks!

[joncinque]

Could we add a cargo hack check --each-feature step in CI to catch this? --feature-powerset would likely be overkill, but it's possible too. What do you think?

[grod220]

@joncinque we currently have --feature-powerset but the issue is that --all-targets causes cargo to resolve dev dependencies which enable all features and not allow them to run in isolation.

I'm wondering if that self-referencing dev dependency pattern is idiomatic. I think we can get this coverage by:

1. Dropping the self dep:

- [dev-dependencies]
- spl-collections = { path = ".", features = ["borsh", "wincode"] }

2. Replace test-%:

test-%:
-	SBF_OUT_DIR=$(PWD)/target/deploy cargo $(nightly) test --manifest-path $(call make-path,$*)/Cargo.toml $(ARGS)
+	SBF_OUT_DIR=$(PWD)/target/deploy cargo $(nightly) hack test --each-feature --tests --manifest-path $(call make-path,$*)/Cargo.toml $(ARGS)

3. Make test imports feature gated

#[cfg(test)]
mod tests {
+    #[cfg(any(feature = "borsh", feature = "wincode"))]
     use super::*;

+    #[cfg(feature = "borsh")]
     use borsh::{BorshDeserialize, BorshSerialize};

+    #[cfg(feature = "wincode")]
     use wincode::WriteError;

Hmm. But that puts cargo-hack as a dep on the test-% action. I wonder if we should add support in solana-actions for feature-tests-%.

Thoughts?

[joncinque]

How about we just drop --all-targets on the powerset check?

[febo]

Hmmm, yeah, that seems to be the issue. Weird that it would resolve dependencies for all targets together.

An alternative is not to use --all-targets and run each target individuallly – e.g.:

cargo hack check --feature-powerset --manifest-path collections/Cargo.toml --lib
cargo hack check --feature-powerset --manifest-path collections/Cargo.toml --tests
... and so on

We can tweak the script to go over all the targets that we want.

[grod220]

How about we just drop --all-targets on the powerset check?

That will grant feature isolation for the build, but then we lose the compile checks on the feature-gated tests.

But if we keep the self-referencing dev deps, the test-% command covers those feature-gated test builds anyway (not in isolation though).

That would make the ci/cd have:

1. Feature isolated checks for library build
2. One cargo test run with all features enabled at once

If we are cool with that, then that's a simple fix. If we find that tests should also have feature isolation, we'd need a solution that runs cargo hack test --each-feature --tests.

An alternative is not to use --all-targets and run each target individuallly

Would be nice somehow to not have to maintain that list in the makefile and keep package lists in .github/workflows/main.yml.

[joncinque]

Sorry if this is a silly question, but what do we get out of feature isolation in tests?

[grod220]

Sorry if this is a silly question, but what do we get out of feature isolation in tests?

A test that uses multiple features that should not be used together? I can't think of a specific case but perhaps someone writes a test that implicitly relies on everything being turned on when that is not a valid feature set (?). Maybe it's not a real issue though 🤔

[joncinque]

#189 for the fix