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

Surface the failing command's name from the build error dump #253

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Sep 17, 2024

Because Cargo's formatting of build errors is very noisy users struggle to understand what is causing the build to fail.

Here's a recent case where the failure reason was simple, but the user could not see it through all the noise.


    Compiling openssl-sys v0.9.103 (~/openssl-sys/openssl-sys)
+The following warnings were emitted during compilation:
+
+warning: openssl-sys@0.9.103: configuring OpenSSL build: Command 'perl' not found. Is perl installed?
+
 error: failed to run custom build command for `openssl-sys v0.9.103 (~/openssl-sys/openssl-sys)`
 note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.
 
 Caused by:
   process didn't exit successfully: `~/openssl-sys/target/debug/build/openssl-sys-60b6752181d583d2/build-script-main` (exit status: 101)
 
 [...]

   --- stderr
-   thread 'main' panicked at ~/openssl-src-rs/src/lib.rs:578:14:
+   thread 'main' panicked at ~/openssl-sys/build/find_vendored.rs:18:39:
 
 
 
   Error configuring OpenSSL build:
-      Command: cd "~/openssl-sys/target/debug/build/openssl-sys-8f90d3753aa6037e/out/openssl-build/build/src" && env -u CROSS_COMPILE AR="ar" CC="cc" RANLIB="ranlib" "perl" "./Configure" "--prefix=~/openssl-sys/target/debug/build/openssl-sys-8f90d3753aa6037e/out/openssl-build/install" "--openssldir=/usr/local/ssl" "no-dso" "no-shared" "no-ssl3" "no-tests" "no-comp" "no-zlib" "no-zlib-dynamic" "--libdir=lib" "no-md2" "no-rc5" "no-weak-ssl-ciphers" "no-camellia" "no-idea" "no-seed" "darwin64-arm64-cc" "-nologo" "-MD" "-O2" "-Z7" "-Brepro" "-mmacosx-version-min=11.0"
-      Failed to execute: No such file or directory (os error 2)
+      Command 'perl' not found. Is perl installed?
+      Command failed: cd "~/openssl-sys/target/debug/build/openssl-sys-3ae48db4843d8fdf/out/openssl-build/build/src" && env -u CROSS_COMPILE AR="ar" CC="cc" RANLIB="ranlib" "perl" "./Configure" "--prefix=~/openssl-sys/target/debug/build/openssl-sys-3ae48db4843d8fdf/out/openssl-build/install" "--openssldir=/usr/local/ssl" "no-shared" "no-ssl3" "no-tests" "no-comp" "no-zlib" "no-zlib-dynamic" "--libdir=lib" "no-md2" "no-rc5" "no-weak-ssl-ciphers" "no-camellia" "no-idea" "no-seed" "darwin64-arm64-cc" "-nologo" "-MD" "-O2" "-Z7" "-Brepro" "-mmacosx-version-min=11.0"
  • Cleverness of the Debug impl of Command obscures the command name that has been run. This change prints the command name alone.
  • NotFound error is handled with a dedicated message.
  • Uses cargo:warning to also print the message above the stdout and stderr dumps.

@kornelski
Copy link
Contributor Author

I've also added try_build, so that I can make openssl-sys report the error without a noisy backtrace.

@bjorn3
Copy link

bjorn3 commented Sep 19, 2024

Instead of using panic, maybe using eprintln!() + std::process::exit(1) would work? openssl-src is probably not called in any context where recovering from an unexpected build error is necessary, and you added try_build for cases where build errors are expected to happen.

@kornelski
Copy link
Contributor Author

Exiting directly without a panic would be cleaner, but I'm leaving that to the caller of try_build – I'm adding such exits to openssl-sys.

Copy link
Owner

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! If try_build is being introduced could this result-ify all the unwraps/expects? The existence of try_build and build implies that one never panics while the other might, so I'd prefer to head off confusion.

Also I agree with @bjorn3 that if the goal is a "clean exit" for the build method, could that be updated to print+exit instead of a formatted panic?

@kornelski
Copy link
Contributor Author

I've updated it with a clean exit, and converted other panics into errors.

src/lib.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Owner

Thanks for this!

@alexcrichton alexcrichton merged commit 09f1a28 into alexcrichton:main Sep 20, 2024
17 checks passed
@kornelski kornelski deleted the big-cmd branch September 20, 2024 00:42
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