Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

give more helpful error messages for SGX_MODE and IAS_MODE settings #3164

Merged
merged 5 commits into from
Feb 24, 2023

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Feb 24, 2023

Many projects submodule mobilecoin.git and do not have it's BUILD.md document which describes how to set these variables.

It frequently happens that newly onboarded developers, or partners like copper, run into a build error that looks like this:

   Compiling libp2p-plaintext v0.37.0
   Compiling libp2p-floodsub v0.40.1
   Compiling slog-envlogger v2.2.0
error: failed to run custom build command for `mc-attest-core v4.0.2 (/home/chris/mobilecoinofficial/deqs/mobilecoin/attest/core)`

Caused by:
  process didn't exit successfully: `/home/chris/mobilecoinofficial/deqs/target/debug/build/mc-attest-core-fa6d778110b3deb5/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not parse SGX environment: Variable(NotPresent)', mobilecoin/attest/core/build.rs:8:41
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This error doesn't contain enough information for them to understand and fix the problem, so they end up asking us for help.

We can follow the rust tradition of trying to make build-time errors contain enough information to understand how to address the problem.

This commit tries to make the message say what variable was not present, and also contain a link to the mobilecoin.git BUILD.md so that they can learn more.

Many projects submodule mobilecoin.git and do not have it's BUILD.md
document which describes how to set these variables.

It frequently happens that newly onboarded developers, or partners
like copper, run into a build error that looks like this:

```
   Compiling libp2p-plaintext v0.37.0
   Compiling libp2p-floodsub v0.40.1
   Compiling slog-envlogger v2.2.0
error: failed to run custom build command for `mc-attest-core v4.0.2 (/home/chris/mobilecoinofficial/deqs/mobilecoin/attest/core)`

Caused by:
  process didn't exit successfully: `/home/chris/mobilecoinofficial/deqs/target/debug/build/mc-attest-core-fa6d778110b3deb5/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not parse SGX environment: Variable(NotPresent)', mobilecoin/attest/core/build.rs:8:41
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

This error doesn't contain enough information for them to understand
and fix the problem, so they end up asking us for help.

We can follow the rust tradition of trying to make build-time errors
contain enough information to understand how to address the problem.

This commit tries to make the message say what variable was not present,
and also contain a link to the mobilecoin.git BUILD.md so that they can
learn more.
@cbeck88 cbeck88 requested review from jcape, nick-mobilecoin and a team February 24, 2023 01:06
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a step in the right direction, but it still misses the intent in the PR description.

I'm guessing expect() uses debug instead of display, kind of surprised but maybe it's a compile time optimization.

error: failed to run custom build command for `mc-attest-core v4.0.2 (/home/nick/git/mobilecoin/attest/core)`

Caused by:
  process didn't exit successfully: `/home/nick/git/mobilecoin/target/debug/build/mc-attest-core-765438a6f0b96ded/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not parse SGX environment: Variable("REALLY", NotPresent)', attest/core/build.rs:8:41
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
error: failed to run custom build command for `mc-attest-ake v4.0.2 (/home/nick/git/mobilecoin/attest/ake)`

Caused by:
  process didn't exit successfully: `/home/nick/git/mobilecoin/target/debug/build/mc-attest-ake-2d6c54bb8b6bee8d/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not parse SGX environment: Variable("REALLY", NotPresent)', attest/ake/build.rs:8:41
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtracemobilecoin git:(sgx_mode_error_messages) ✗ rg "Could not parse SGX"
consensus/enclave/build.rs
19:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

consensus/enclave/trusted/build.rs
21:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/view/enclave/build.rs
19:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/view/enclave/trusted/build.rs
21:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/ingest/enclave/build.rs
19:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/ingest/enclave/trusted/build.rs
21:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/ledger/enclave/build.rs
19:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

fog/ledger/enclave/trusted/build.rs
21:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

attest/core/build.rs
8:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

attest/ake/build.rs
8:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

attest/verifier/build.rs
281:    let sgx = SgxEnvironment::new(&env).expect("Could not parse SGX environment");

@cbeck88
Copy link
Contributor Author

cbeck88 commented Feb 24, 2023

damn, thank you. i guess i will not derive Debug, and make Debug forward to Display

@nick-mobilecoin
Copy link
Collaborator

It looks like all usages of SgxEnvironment::new() chain with expect() so maybe new() can be updated to panic and clean up the error message there.
Another thought is to remove new and implement TryFrom and From since new only has one parameter.
Either way I think the 11 instances of the same expect hint that maybe they should be conslidated

@nick-mobilecoin
Copy link
Collaborator

damn, thank you. i guess i will not derive Debug, and make Debug forward to Display

or that works too :)

Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now

Caused by:
  process didn't exit successfully: `/home/nick/git/mobilecoin/target/debug/build/mc-attest-core-765438a6f0b96ded/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not parse SGX environment: There was an error reading an environment variable REALLY: environment variable not found. Please see https://github.com/mobilecoinfoundation/mobilecoin/blob/master/BUILD.md#build-configuration for more information.', attest/core/build.rs:8:41

@github-actions
Copy link

❌ Heads up, I tried building full-service using this branch and it failed.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / full-service or by clicking here.

@cbeck88 cbeck88 requested a review from a team February 24, 2023 08:20
@cbeck88 cbeck88 enabled auto-merge (squash) February 24, 2023 17:22
Copy link
Contributor

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, the error messages are much more helpful and clear.

@cbeck88 cbeck88 merged commit 94425c7 into master Feb 24, 2023
@cbeck88 cbeck88 deleted the sgx_mode_error_messages branch February 24, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants