Add solana-crate-features workaround to avoid cargo feature thrashing (…

…solana-labs#5904)

automerge

crate-features/.gitignore

@@ -0,0 +1,2 @@
+/target/
+/farf/

crate-features/0001-Print-package-features.patch

@@ -0,0 +1,44 @@
+From 64514728f549719b8b42af1570404d5f51730e8c Mon Sep 17 00:00:00 2001
+From: Michael Vines <mvines@gmail.com>
+Date: Fri, 13 Sep 2019 17:34:43 -0700
+Subject: [PATCH] Print package features
+
+---
+ src/cargo/core/compiler/context/compilation_files.rs | 8 ++++++++
+ src/cargo/core/compiler/fingerprint.rs               | 2 +-
+ 2 files changed, 9 insertions(+), 1 deletion(-)
+
+diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs
+index ed3232ea..c98da4ca 100644
+--- a/src/cargo/core/compiler/context/compilation_files.rs
++++ b/src/cargo/core/compiler/context/compilation_files.rs
+@@ -591,5 +591,13 @@ fn compute_metadata<'a, 'cfg>(
+     if let Ok(ref channel) = __cargo_default_lib_metadata {
+         channel.hash(&mut hasher);
+     }
++
++    eprintln!(
++        "package {}: {} #{} features={:?}",
++        unit.pkg.package_id(),
++        unit.target,
++        hasher.finish(),
++        bcx.resolve.features_sorted(unit.pkg.package_id()),
++    );
+     Some(Metadata(hasher.finish()))
+ }
+diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs
+index 3738bcdd..de78ffb4 100644
+--- a/src/cargo/core/compiler/fingerprint.rs
++++ b/src/cargo/core/compiler/fingerprint.rs
+@@ -830,7 +830,7 @@ impl Fingerprint {
+             // for a discussion of why it's `>` see the discussion about #5918
+             // below in `find_stale`.
+             if dep_mtime > max_mtime {
+-                log::info!("dependency on `{}` is newer than we are", dep.name);
++                eprintln!("dependency on `{}` is newer than we are", dep.name);
+                 return Ok(());
+             }
+         }
+-- 
+2.20.1 (Apple Git-117)
+

crate-features/Cargo.toml

@@ -0,0 +1,25 @@
+[package]
+name = "solana-crate-features"
+version = "0.19.0-pre0"
+authors = ["Solana Maintainers <maintainers@solana.com>"]
+repository = "https://github.com/solana-labs/solana"
+homepage = "https://solana.com/"
+license = "Apache-2.0"
+edition = "2018"
+
+[dependencies]
+backtrace = { version = "0.3.33", features = ["serialize-serde"] }
+bytes = { version = "0.4.12", features = ["either"] }
+cc = { version = "1.0.45", features = ["jobserver", "num_cpus", "parallel"]}
+curve25519-dalek = { version = "1.1.3" }
+either= { version = "1.5.3" }
+failure = { version = "0.1.5" }
+lazy_static = { version = "1.4.0", features = ["spin", "spin_no_std"] }
+libc = { version = "0.2.62", features = ["extra_traits"] }
+rand_chacha = { version = "0.1.1" }
+regex-syntax = { version = "0.6.12" }
+serde = { version = "1.0.100", features = ["rc"] }
+solana-ed25519-dalek = { version = "0.2.0", features = ["serde"] }
+syn = { version = "1.0.3", features = ["extra-traits", "full"] }
+ureq = { version = "0.11.0", features = ["json"] }
+winapi = { version = "0.3.8", features=["basetsd", "consoleapi", "errhandlingapi", "fileapi", "handleapi", "impl-debug", "impl-default", "knownfolders", "libloaderapi", "memoryapi", "minwinbase", "minwindef", "ntdef", "ntsecapi", "ntstatus", "objbase", "processenv", "processthreadsapi", "profileapi", "shlobj", "std", "synchapi", "sysinfoapi", "timezoneapi", "utilapiset", "winbase", "wincon", "windef", "winerror", "winnls", "winnt", "winreg", "winsock2", "winuser", "ws2def", "ws2ipdef", "ws2tcpip", "wtypesbase"] }

crate-features/README.md

@@ -0,0 +1,58 @@
+
+Cargo (as of 1.37) does not nicely handle the scenario where features of a
+dependent crate of multiple other crates in the tree vary.
+
+To illustrate the problem consider crates A, B and C arranged as follows:
+* Crate A and B are both members of a Cargo virtual manifest
+* Crate C provides two features, F1 and F2
+* Crate A requests feature F1 of C, crate B requests F2 of C
+
+When crate A and B are built together, `cargo` builds C with both feature F1 and
+F2 enabled (the union of all enabled features).  However when A or B are built
+individually, `cargo` builds C with only feature F1 or F2 enabled.
+
+Unfortunately in all these cases, `cargo` will build crate C in the same target
+location and the outputs for C will be recreated every time the features for crate C
+change.
+
+From a clean workspace, building A individually first will cause C to be built
+as expected.  Then build B individually and C will be re-built because F2 was
+enabled instead of F1.  Now rebuild A and observe that C will be re-built again
+because F1 was re-enabled.
+
+In practice this problem is much less obvious as both A and B likely to not have
+a direct dependency on C, indirectly causing rebuilds of numerous other crates as well.
+
+The `solana-crate-features` offers a workaround to this "feature thrashing"
+problem by explicitly declaring all "C-like crates" with the union of all features
+that any other crate in the tree (either explicitly or implicitly) enable.  All
+crates in the Solana source tree should depend on `solana-crate-features`.
+
+### Adding new dependent crates
+When unnecessary `cargo` rebuilds are observed, the first step is to figure what
+dependent crate is suffering from feature thrashing.
+
+This information is not readily available from the stock `cargo` program, so use
+the following steps to produce a custom `cargo` program that will output the
+necessary feature information to stderr during a build:
+```bash
+$ git clone git@github.com:rust-lang/cargo.git -b rust-1.37.0
+$ cd cargo
+$ git apply 0001-Print-package-features.patch
+$ cargo build
+$ mv ~/.cargo/bin/cargo ~/.cargo/bin/cargo.org
+$ cp ./target/debug/cargo ~/.cargo/bin/cargo
+```
+
+Rebuild with the custom `cargo` and search for indications of crates getting
+built with different features (repeated runs of `./scripts/cargo-install-all.sh`
+work great for this).  When the problematic crate is identified, add it as a
+dependency of `solana-crate-features` with the union of all observed enabled
+features for that crate.
+
+### Appendix
+This command will enable some additional cargo log output that can help debug
+dependency problems as well:
+```bash
+export CARGO_LOG=cargo::core::compiler::fingerprint=info
+```

crate-features/src/lib.rs

@@ -0,0 +1 @@
+

multinode-demo/common.sh

@@ -55,7 +55,7 @@ else
     if [[ -n $NDEBUG ]]; then
       maybe_release=--release
     fi
-    declare manifest_path="--manifest-path=$SOLANA_ROOT/$crate/Cargo.toml"
+    declare manifest_path="--manifest-path=$SOLANA_ROOT/Cargo.toml"
     printf "cargo run $manifest_path $maybe_release $maybe_package --bin %s %s -- " "$program" "$features"
   }
 fi

net/net.sh

@@ -125,7 +125,6 @@ internalNodesLamports=
 maybeNoSnapshot=""
 maybeSkipLedgerVerify=""
 maybeDisableAirdrops=""
-buildProfile="--release"
 debugBuild=false
 doBuild=true
 
@@ -276,8 +275,6 @@ while getopts "h?T:t:o:f:rD:c:Fn:i:d" opt "${shortArgs[@]}"; do
   esac
 done
 
-$debugBuild && buildProfile=""
-
 loadConfigFile
 
 if [[ -n $numFullnodesRequested ]]; then
@@ -332,9 +329,15 @@ build() {
       # shellcheck source=/dev/null
       source target/perf-libs/env.sh
     fi
+
+    buildVariant=
+    if $debugBuild; then
+      buildVariant=debug
+    fi
+
     $MAYBE_DOCKER bash -c "
       set -ex
-      scripts/cargo-install-all.sh farf \"$cargoFeatures\" \"$buildProfile\"
+      scripts/cargo-install-all.sh farf \"$cargoFeatures\" \"$buildVariant\"
       if [[ -n \"$customPrograms\" ]]; then
         scripts/cargo-install-custom-programs.sh farf $customPrograms
       fi

scripts/cargo-install-all.sh

@@ -16,16 +16,18 @@ if [[ -z $1 ]]; then
 fi
 
 installDir="$(mkdir -p "$1"; cd "$1"; pwd)"
+cargo=cargo
 cargoFeatures="$2"
+debugBuild="$3"
 
-buildProfile="$3"
-if [[ -n "$buildProfile" ]]; then
-  binTargetDir="release"
-else
-  binTargetDir="debug"
+buildVariant=release
+maybeReleaseFlag=--release
+if [[ -n "$debugBuild" ]]; then
+  maybeReleaseFlag=
+  buildVariant=debug
 fi
 
-echo "Install location: $installDir"
+echo "Install location: $installDir ($buildVariant)"
 
 cd "$(dirname "$0")"/..
 
@@ -34,36 +36,45 @@ SECONDS=0
 (
   set -x
   # shellcheck disable=SC2086 # Don't want to double quote $rust_version
-  cargo $rust_version build --all $buildProfile --features="$cargoFeatures"
+  $cargo $rust_version build --all $maybeReleaseFlag --features="$cargoFeatures"
 )
 
-BIN_CRATES=(
-  drone
-  gossip
-  install
-  keygen
-  ledger-tool
-  replicator
-  validator
-  cli
-  bench-exchange
-  bench-tps
+BINS=(
+  solana-drone
+  solana-gossip
+  solana-install
+  solana-install-init
+  solana-keygen
+  solana-ledger-tool
+  solana-replicator
+  solana-validator
+  solana
+  solana-bench-exchange
+  solana-bench-tps
 )
 
 #XXX: Ensure `solana-genesis` is built LAST!
 # See https://github.com/solana-labs/solana/issues/5826
-BIN_CRATES+=( "genesis" )
+BINS+=(solana-genesis)
 
-for crate in "${BIN_CRATES[@]}"; do
-  (
-    set -x
-    # shellcheck disable=SC2086 # Don't want to double quote $rust_version
-    cargo $rust_version install --force --path "$crate" --root "$installDir" --features="$cargoFeatures"
-  )
+binArgs=()
+for bin in "${BINS[@]}"; do
+  binArgs+=(--bin "$bin")
+done
+
+(
+  set -x
+  # shellcheck disable=SC2086 # Don't want to double quote $rust_version
+  $cargo $rust_version build $maybeReleaseFlag "${binArgs[@]}" --features="$cargoFeatures"
+)
+
+mkdir -p "$installDir/bin"
+for bin in "${BINS[@]}"; do
+  cp -fv "target/$buildVariant/$bin" "$installDir"/bin
 done
 
 for dir in programs/*; do
-  for program in echo target/$binTargetDir/deps/libsolana_"$(basename "$dir")".{so,dylib,dll}; do
+  for program in echo target/$buildVariant/deps/libsolana_"$(basename "$dir")".{so,dylib,dll}; do
     if [[ -f $program ]]; then
       mkdir -p "$installDir/bin/deps"
       rm -f "$installDir/bin/deps/$(basename "$program")"
@@ -72,5 +83,4 @@ for dir in programs/*; do
   done
 done
 
-du -a "$installDir"
 echo "Done after $SECONDS seconds"

sdk/Cargo.toml

@@ -23,6 +23,7 @@ default = [
     "solana-ed25519-dalek",
     "solana-logger",
     "untrusted",
+    "solana-crate-features"
 ]
 
 [dependencies]
@@ -46,4 +47,5 @@ serde_json = { version = "1.0.40", optional = true }
 sha2 = "0.8.0"
 solana-ed25519-dalek = { version = "0.2.0", optional = true }
 solana-logger = { path = "../logger", version = "0.19.0-pre0", optional = true }
-untrusted = { version = "0.7.0", optional = true }
+untrusted = { version = "0.7.0", optional = true }
+solana-crate-features = { path = "../crate-features", version = "0.19.0-pre0", optional = true }