[x] add a linter for explicitly listing out test-only crates

Test-only crates have more relaxed rules -- for example, they don't need to
have overlay features defined.

Also update the list of default workspace members to cover everything in the
production build.

Closes: aptos-labs#3973
Approved by: bmwill

Cargo.toml

@@ -127,14 +127,26 @@ members = [
     "vm-validator",
 ]
 
-# NOTE: These members should never include crates that require fuzzing
-# features or test features. These are the crates we want built with no extra
+# NOTE: default-members is the complete list of binaries that form the "production Libra codebase". These members should
+# never include crates that require fuzzing features or test features. These are the crates we want built with no extra
 # test-only code included.
+#
+# For more, see the "Conditional compilation for tests" section in documentation/coding_guidelines.md.
 default-members = [
+    "config/config-builder",
+    "config/management",
+    "client/libra-dev",
+    "execution/db-bootstrapper",
     "testsuite/cli",
     "language/compiler",
+    "language/move-prover",
+    "language/tools/disassembler",
+    "language/tools/genesis-viewer",
     "language/tools/vm-genesis",
     "libra-node",
+    "secure/key-manager",
+    "storage/backup/backup-cli",
+    "storage/inspector",
     "storage/storage-service",
 ]
 

config/management/Cargo.toml

@@ -32,9 +32,13 @@ storage-interface = { path = "../../storage/storage-interface", version = "0.1.0
 transaction-builder = { path = "../../language/transaction-builder", version = "0.1.0" }
 vm-genesis = { path = "../../language/tools/vm-genesis", version = "0.1.0" }
 
+# libra-swarm is actually a dev-dependency, but because libra-management is a default workspace member,
+# cargo's V1 resolver will make it take part in feature resolution. Setting it as optional works, but it
+# needs to be moved into the [dependencies] section for that to work.
+libra-swarm = { path = "../../testsuite/libra-swarm", version = "0.1.0", optional = true }
+
 [dev-dependencies]
 config-builder = { path = "../config-builder", version = "0.1.0" }
-libra-swarm = { path = "../../testsuite/libra-swarm", version = "0.1.0" }
 
 [features]
-testing = ["libra-secure-storage/testing"]
+fuzzing = ["libra-secure-storage/fuzzing"]

devtools/x/src/config.rs

@@ -44,6 +44,8 @@ pub struct WorkspaceConfig {
     pub banned_direct_deps: HashMap<String, String>,
     /// Overlay config in this workspace
     pub overlay: OverlayConfig,
+    /// Test-only config in this workspace
+    pub test_only: TestOnlyConfig,
 }
 
 #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
@@ -62,6 +64,13 @@ pub struct OverlayConfig {
     pub features: Vec<String>,
 }
 
+#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
+#[serde(rename_all = "kebab-case")]
+pub struct TestOnlyConfig {
+    /// A list of test-only members
+    pub members: Vec<PathBuf>,
+}
+
 #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
 #[serde(rename_all = "kebab-case")]
 pub struct Clippy {

devtools/x/src/lint/guppy.rs

@@ -3,12 +3,16 @@
 
 //! Project and package linters that run queries on guppy.
 
-use crate::config::{EnforcedAttributesConfig, OverlayConfig};
+use crate::{
+    config::{EnforcedAttributesConfig, OverlayConfig, TestOnlyConfig},
+    lint::toml::toml_mismatch_message,
+};
 use guppy::{graph::feature::FeatureFilterFn, Version};
 use std::{
     collections::{BTreeMap, HashMap},
     ffi::OsStr,
     iter,
+    path::Path,
 };
 use x_lint::prelude::*;
 
@@ -66,6 +70,69 @@ impl<'cfg> ProjectLinter for BannedDirectDeps<'cfg> {
     }
 }
 
+/// Ensure that the list of test-only crates is up to date.
+#[derive(Debug)]
+pub struct TestOnlyMembers<'cfg> {
+    config: &'cfg TestOnlyConfig,
+}
+
+impl<'cfg> TestOnlyMembers<'cfg> {
+    pub fn new(config: &'cfg TestOnlyConfig) -> Self {
+        Self { config }
+    }
+}
+
+impl<'cfg> Linter for TestOnlyMembers<'cfg> {
+    fn name(&self) -> &'static str {
+        "test-only-members"
+    }
+}
+
+impl<'cfg> ProjectLinter for TestOnlyMembers<'cfg> {
+    fn run<'l>(
+        &self,
+        ctx: &ProjectContext<'l>,
+        out: &mut LintFormatter<'l, '_>,
+    ) -> Result<RunStatus<'l>> {
+        // Set of test-only members is all workspace members minus default ones.
+        let pkg_graph = ctx.package_graph()?;
+        let workspace = pkg_graph.workspace();
+        let default_members = ctx.default_workspace_members()?;
+        let mut expected: Vec<_> = workspace
+            .members()
+            .filter_map(|(path, package)| {
+                if default_members.contains(package.id()) {
+                    None
+                } else {
+                    Some(path)
+                }
+            })
+            .collect();
+        expected.sort();
+
+        if expected != self.config.members {
+            // Create the expected TestOnlyConfig struct.
+            let expected = TestOnlyConfig {
+                members: expected
+                    .into_iter()
+                    .map(|path| path.to_path_buf())
+                    .collect(),
+            };
+            out.write_kind(
+                LintKind::File(Path::new("x.toml")),
+                LintLevel::Error,
+                toml_mismatch_message(
+                    &expected,
+                    self.config,
+                    "test-only member list not canonical",
+                )?,
+            );
+        }
+
+        Ok(RunStatus::Executed)
+    }
+}
+
 /// Enforce attributes on workspace crates.
 #[derive(Debug)]
 pub struct EnforcedAttributes<'cfg> {

devtools/x/src/lint/mod.rs

@@ -23,6 +23,7 @@ pub fn run(args: Args, xctx: XContext) -> crate::Result<()> {
     let project_linters: &[&dyn ProjectLinter] = &[
         &guppy::BannedDirectDeps::new(&workspace_config.banned_direct_deps),
         &guppy::DirectDepDups,
+        &guppy::TestOnlyMembers::new(&workspace_config.test_only),
     ];
 
     let package_linters: &[&dyn PackageLinter] = &[

devtools/x/src/lint/toml.rs

@@ -51,22 +51,13 @@ impl ContentLinter for RootToml {
         };
 
         if workspace.members != expected.members {
-            // Serialize and compute the diff between them.
-            let expected = to_toml_string(&expected)
-                .map_err(|err| SystemError::ser("serializing expected workspace members", err))?;
-            let actual = to_toml_string(&workspace)
-                .map_err(|err| SystemError::ser("serializing actual workspace members", err))?;
-            // TODO: print out a context diff instead of the full diff.
-            out.write_kind(
-                LintKind::File(Path::new("Cargo.toml")),
+            out.write(
                 LintLevel::Error,
-                format!(
-                    "workspace member list not canonical:\n\n{}",
-                    PrettyDifference {
-                        expected: &expected,
-                        actual: &actual
-                    }
-                ),
+                toml_mismatch_message(
+                    &expected,
+                    &workspace,
+                    "workspace member list not canonical",
+                )?,
             );
         }
 
@@ -76,6 +67,27 @@ impl ContentLinter for RootToml {
     }
 }
 
+/// Creates a lint message indicating the differences between the two TOML structs.
+pub(super) fn toml_mismatch_message<T: Serialize>(
+    expected: &T,
+    actual: &T,
+    header: &str,
+) -> Result<String> {
+    let expected = to_toml_string(expected)
+        .map_err(|err| SystemError::ser("serializing expected workspace members", err))?;
+    let actual = to_toml_string(actual)
+        .map_err(|err| SystemError::ser("serializing actual workspace members", err))?;
+    // TODO: print out a context diff instead of the full diff.
+    Ok(format!(
+        "{}:\n\n{}",
+        header,
+        PrettyDifference {
+            expected: &expected,
+            actual: &actual
+        }
+    ))
+}
+
 /// Serializes some data to toml using this project's standard code style.
 fn to_toml_string<T: Serialize>(data: &T) -> Result<String, ser::Error> {
     let mut dst = String::with_capacity(128);

documentation/coding_guidelines.md

@@ -274,7 +274,11 @@ References:
 * [An introduction to property-based testing](https://fsharpforfunandprofit.com/posts/property-based-testing/)
 * [Choosing properties for property-based testing](https://fsharpforfunandprofit.com/posts/property-based-testing-2/)
 
-*Conditional compilation of tests*
+*Fuzzing*
+
+Libra contains harnesses for fuzzing crash-prone code like deserializers, using [`libFuzzer`](https://llvm.org/docs/LibFuzzer.html) through [`cargo fuzz`](https://rust-fuzz.github.io/book/cargo-fuzz.html). For more examples, see the `testsuite/libra_fuzzer` directory.
+
+### Conditional compilation of tests
 
 Libra [conditionally
 compiles](https://doc.rust-lang.org/stable/reference/conditional-compilation.html)
@@ -286,38 +290,70 @@ in tests/benchmarks](https://github.com/rust-lang/cargo/issues/2911), we rely on
 conditions to perform this conditional compilation:
 - the test flag, which is activated by dependent test code in the same crate
   as the conditional test-only code.
-- the "fuzzing" custom feature, which is used to enable fuzzing and testing
+- the `fuzzing` custom feature, which is used to enable fuzzing and testing
 related code in downstream crates. Note that this must be passed explicitly to
 `cargo xtest` and `cargo x bench`. Never use this in `[dependencies]` or
 `[dev-dependencies]` unless the crate is only for testing, otherwise Cargo's
 feature unification may pollute production code with the extra testing/fuzzing code.
 
-As a consequence, it is recommended that you set up your test-only code in the following fashion. For the sake of example, we'll consider you are defining a test-only helper function `foo` in `foo_crate`:
-1. Define the "testing" flag in `foo_crate/Cargo.toml` and make it non-default:
-    ```
+As a consequence, it is recommended that you set up your test-only code in the following fashion.
+
+**For production crates:**
+
+Production crates are defined as the set of crates that create externally published artifacts, e.g. the Libra validator,
+the Move compiler, and so on.
+
+For the sake of example, we'll consider you are defining a test-only helper function `foo` in `foo_crate`:
+
+1. Define the `fuzzing` flag in `foo_crate/Cargo.toml` and make it non-default:
+    ```toml
     [features]
     default = []
     fuzzing = []
     ```
 2. Annotate your test-only helper `foo` with both the `test` flag (for in-crate callers) and the `"fuzzing"` custom feature (for out-of-crate callers):
-    ```
+    ```rust
     #[cfg(any(test, feature = "fuzzing"))]
     fn foo() { ... }
     ```
 3. (optional) Use `cfg_attr` to make test-only trait derivations conditional:
-    ```
+    ```rust
     #[cfg_attr(any(test, feature = "testing"), derive(FooTrait))]
     #[derive(Debug, Display, ...)] // inconditional derivations
     struct Foo { ... }
     ```
-4. (optional) Set up feature transitivitity for crates that call crates that have test-only members. Let's say it's the case of `bar_crate`, which, through its test helpers, calls into `foo_crate` to use your test-only `foo`. Here's how you would set up `bar_crate/Cargo.toml`:
-    ```
+4. (optional) Set up feature transitivity for crates that call crates that have test-only members. Let's say it's the case of `bar_crate`, which, through its test helpers, calls into `foo_crate` to use your test-only `foo`. Here's how you would set up `bar_crate/Cargo.toml`:
+    ```toml
     [features]
     default = []
-    testing = ["foo_crate/testing"]
+    fuzzing = ["foo_crate/fuzzing"]
     ```
-5. Update `x/src/test_unit.rs` to run the unit tests passing in the
-   features if needed.
+5. Update `x.toml` to run the unit tests passing in the features if needed.
+
+**Special case:** If a test-only crate (see below) is a dev-dependency of a production crate listed in the root
+`Cargo.toml`'s `default-members`, it needs to be marked optional for feature resolution to work properly. Do this by
+marking the dependency as optional and moving it to the `[dependencies]` section.
+
+```toml
+[dependencies]
+foo_crate = { path = "...", optional = true }
+```
+
+(This is a temporary workaround for a Cargo issue and is expected to be addressed with Cargo's [new feature
+resolver](https://github.com/rust-lang/cargo/pull/7820)).
+
+**For test-only crates:**
+
+Test-only crates do not create published artifacts. They consist of tests, benchmarks or other code that verifies
+the correctness or performance of published artifacts. Test-only crates are explicitly listed in `x.toml`.
+
+These crates do not need to use the above setup. Instead, they can enable the `fuzzing` feature in production crates
+directly.
+
+```toml
+[dependencies]
+foo_crate = { path = "...", features = ["fuzzing"] }
+```
 
 *A final note on integration tests*: All tests that use conditional test-only
 elements in another crate need to activate the "fuzzing" feature through the
@@ -326,11 +362,7 @@ tests](https://doc.rust-lang.org/rust-by-example/testing/integration_testing.htm
 can neither rely on the `test` flag nor do they have a proper `Cargo.toml` for
 feature activation. In the Libra codebase, we therefore recommend that
 *integration tests which depend on test-only code in their tested crate* be
-extracted to their own crate. You can look at `language/vm/serializer_tests`
+extracted to their own test-only crate. See `language/vm/serializer_tests`
 for an example of such an extracted integration test.
 
 *Note for developers*: The reason we use a feature re-export (in the `[features]` section of the `Cargo.toml` is that a profile is not enough to activate the `"fuzzing"` feature flag. See [cargo-issue #291](https://github.com/rust-lang/cargo/issues/2911) for details).
-
-*Fuzzing*
-
-Libra contains harnesses for fuzzing crash-prone code like deserializers, using [`libFuzzer`](https://llvm.org/docs/LibFuzzer.html) through [`cargo fuzz`](https://rust-fuzz.github.io/book/cargo-fuzz.html). For more examples, see the `testsuite/libra_fuzzer` directory.

storage/backup/backup-cli/Cargo.toml

@@ -35,5 +35,7 @@ backup-service = { path = "../backup-service", version = "0.1.0" }
 libra-config = { path = "../../../config", version = "0.1.0" }
 libra-proptest-helpers = { path = "../../../common/proptest-helpers" }
 libra-temppath = { path = "../../../common/temppath", version = "0.1.0" }
-libradb = { path = "../../libradb", version = "0.1.0", features = ["fuzzing"]}
 storage-interface = { path = "../../storage-interface", version = "0.1.0" }
+
+[features]
+fuzzing = ["libradb/fuzzing"]

types/Cargo.toml

@@ -38,7 +38,6 @@ move-core-types = { path = "../language/move-core/types", version = "0.1.0" }
 regex = "1.3.7"
 proptest = "0.9.6"
 proptest-derive = "0.1.2"
-libra-proptest-helpers = { path = "../common/proptest-helpers", version = "0.1.0" }
 libra-prost-test-helpers = { path = "../common/prost-test-helpers", version = "0.1.0" }
 serde_json = "1.0.53"
 

x.toml

@@ -20,3 +20,45 @@ lazy_static = "use once_cell::sync::Lazy instead"
 
 [workspace.overlay]
 features = ["fuzzing"]
+
+# This is a list of test-only members. These are workspace members that do not form part of the main
+# Libra production codebase, and are only used to verify correctness and/or performance.
+#
+# *** IMPORTANT ***
+#
+# Published developer tools (e.g. Move compiler) ARE part of the production Libra codebase.
+# They should be listed in the root Cargo.toml's default-members, not here!
+#
+# Before adding a new crate to this list, ensure that it is *actually* test-only. If not, add it
+# (or a crate that depends on it) to the root Cargo.toml's default-members list!
+#
+# For more, see the "Conditional compilation for tests" section in documentation/coding_guidelines.md.
+[workspace.test-only]
+members = [
+    "common/proptest-helpers",
+    "common/prost-test-helpers",
+    "common/retrier",
+    "common/workspace-builder",
+    "devtools/x",
+    "devtools/x-core",
+    "devtools/x-lint",
+    "execution/executor-benchmark",
+    "execution/executor-test-helpers",
+    "language/benchmarks",
+    "language/bytecode-verifier/bytecode-verifier-tests",
+    "language/bytecode-verifier/invalid-mutations",
+    "language/e2e-tests",
+    "language/functional-tests",
+    "language/ir-testsuite",
+    "language/move-prover/test-utils",
+    "language/tools/test-generation",
+    "language/tools/utils",
+    "language/vm/serializer-tests",
+    "network/socket-bench-server",
+    "testsuite",
+    "testsuite/cluster-test",
+    "testsuite/generate-format",
+    "testsuite/libra-fuzzer",
+    "testsuite/libra-fuzzer/fuzz",
+    "testsuite/libra-swarm",
+]