0LNetworkCommunity / libra-framework

9 stars 33 forks source link

Build usable diem-node binary in libra-framework project #310

Open dboreham opened 3 months ago

dboreham commented 3 months ago

diem-node is a package dependency in the libra-framework project. It (alone) can be built from within the project root directory with:

$ cargo build -p diem-node

The resulting binary is created here:

ls target/debug/diem-node
target/debug/diem-node

However this binary file is not executable:

$ ./target/debug/diem-node
thread 'main' panicked at /home/david/.cargo/git/checkouts/diem-7dee55d666bb1ba0/668b495/diem-move/diem-vm/src/natives.rs:57:5:

diem-node was compiled with feature flags that shouldn't be enabled.

This is caused by cargo's feature unification.
When you compile two crates with a shared dependency, if one enables a feature flag for the dependency, then it is also enabled for the other crate.

PLEASE RECOMPILE DIEM-NODE SEPARATELY using the following command:
    cargo build --package diem-node

Task is to resolve this, generating instead an executable...executable.

dboreham commented 3 months ago

Some discussion on this here: https://github.com/rust-lang/cargo/issues/10489

dboreham commented 3 months ago

Narrowing in on what it means by "feature flags that shouldn't be enabled":

stack backtrace:
   0: rust_begin_unwind
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_fmt
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:72:14
   2: core::panicking::panic_display
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:262:5
   3: diem_vm::natives::assert_no_test_natives::panic_cold_display
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panic.rs:99:13
   4: diem_vm::natives::assert_no_test_natives
             at /home/david/.cargo/git/checkouts/diem-7dee55d666bb1ba0/668b495/diem-move/diem-vm/src/natives.rs:57:5
   5: diem_node::main
             at /home/david/.cargo/git/checkouts/diem-7dee55d666bb1ba0/668b495/diem-node/src/main.rs:16:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
dboreham commented 3 months ago

Comes from this assertion: https://github.com/0LNetworkCommunity/diem/blob/release/diem-move/diem-vm/src/natives.rs#L57 Called from here: https://github.com/0LNetworkCommunity/diem/blob/release/diem-node/src/main.rs#L16

dboreham commented 3 months ago

Not sure, but I think this is where the features for that code are defined: https://github.com/0LNetworkCommunity/diem/blob/release/diem-move/diem-vm/Cargo.toml#L63

dboreham commented 3 months ago

Error string is in-tree, which is why it doesn't appear in Google searches: https://github.com/0LNetworkCommunity/diem/blob/release/diem-node/src/utils.rs#L14

dboreham commented 3 months ago

Having loaded the context into my LLM, I think the solution is to just specify the "correct" set of features in our Cargo.tomldirective that specifies the dependency on diem-node (here).

Challenge is to determine what that set is.

dboreham commented 3 months ago

Actually that was off-track. Reading the code (which of course has zero comments) in more detail, I think the error message is a red herring. The code is actually checking that we're not trying to run a binary that has test versions of native functions. I assume this is a safety check to stop foot-gunning problems in production?

dboreham commented 3 months ago

Feature that enables the functions it's looking for seems to be #[test_only]

dboreham commented 3 months ago

https://github.com/0LNetworkCommunity/diem/blob/release/Cargo.toml#L216

dboreham commented 3 months ago

https://github.com/0LNetworkCommunity/diem/blob/release/third_party/move/documentation/coding_guidelines.md#conditional-compilation-of-tests

dboreham commented 3 months ago

More digging:

In the diem repo, cargo tree -e features reports this:

├── diem-node feature "default" (command-line)

whereas in the libra-framework repo it reports this:

├── diem-node feature "default" (*)
dboreham commented 3 months ago

Building in libra-framework, the features passed to the compiler for the dependency diem-framework:

     Running `/home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc --crate-name diem_framework --edition=2021 /home/david/.cargo/git/checkouts/diem-7dee55d666bb1ba0/668b495/diem-move/framework/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=169 --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 -C split-debuginfo=unpacked --cfg 'feature="default"' --cfg 'feature="testing"' --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values("default", "fuzzing", "proptest", "proptest-derive", "testing"))' -C 
dboreham commented 3 months ago

In diem repo:

     Running `/home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc --crate-name diem_framework --edition=2021 diem-move/framework/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=169 --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="default"' --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values("default", "fuzzing", "proptest", "proptest-derive", "testing"))' -C metadata=532e9a0dea2c8718
dboreham commented 3 months ago

This is present in the libra build: --cfg 'feature="testing"' While it is not present in the diem build. This seems to be the origin of the problem.

dboreham commented 2 months ago

Tentatively have this diagnosed:

The problem package here is diem-framework. The runtime error occurs because diem-node has a footgun check in main that looks for various test-mode-only native move functions. It finds some and so bails. Those functions in turn are present because the diem-framework package was built with the testing function enabled.

That happens because depending packages have testing enabled:

│       ├── diem-vm feature "testing"
│       │   ├── diem-vm v0.1.0 (https://github.com/0LNetworkCommunity/diem.git?branch=release#668b4953) (*)
│       │   ├── diem-framework feature "testing"
│       │   │   ├── diem-framework v0.1.0 (https://github.com/0LNetworkCommunity/diem.git?branch=release#668b4953) (*)
│       │   │   └── diem-move-stdlib feature "testing"
│       │   │       └── diem-move-stdlib v0.1.1 (https://github.com/0LNetworkCommunity/diem.git?branch=release#668b4953) (*)

This in turn is cause by cargo being dumb. It can't understand how to build two different binaries in the same workspace with different feature sets. So you get features randomly and silently enabled for no good reason.

In our case the reason is that there is also another package (that we didn't ask to build) in the workspace called libra-smoke-tests that depends on a package called smoke-test that in turn depends on diem-framework.

It turns on the testing feature (not surprising since it's a test utility): https://github.com/0LNetworkCommunity/libra-framework/blob/main/smoke-tests/Cargo.toml#L29

Referenced here: https://github.com/0LNetworkCommunity/libra-framework/blob/main/smoke-tests/Cargo.toml#L20 and here: https://github.com/0LNetworkCommunity/diem/blob/668b4953b660fba9313da90f0e7c5f9479f3ad39/testsuite/smoke-test/Cargo.toml#L26

dboreham commented 2 months ago

Some history: https://github.com/rust-lang/cargo/issues/4463 and https://github.com/rust-lang/cargo/issues/10489 and https://github.com/rust-lang/cargo/issues/5364