eminence / procfs

Rust library for reading the Linux procfs filesystem
Other
367 stars 106 forks source link

Add build-script to provide a nicely readable error if on unsupported platform #167

Closed macisamuele closed 2 years ago

macisamuele commented 2 years ago

This PR is aimed to provide a more informative build failure to users that are building procfs library targeting a platform different than linux or android.

The reason for this is that I was eventually planning to contribute to the library and while building it, on MacOS, the library was failing with a very odd error that lead me to look for a possible bug while instead the library is not really meant to be built on MacOS

   Compiling procfs v0.12.0 (/Users/maci/github/procfs)
error[E0433]: failed to resolve: could not find `linux` in `os`
  --> src/process/mod.rs:66:14
   |
66 | use std::os::linux::fs::MetadataExt;
   |              ^^^^^ could not find `linux` in `os`

error[E0425]: cannot find function `stat64` in crate `libc`
   --> src/process/namespaces.rs:25:31
    |
25  |             if unsafe { libc::stat64(cstr.as_ptr(), &mut stat) } != 0 {
    |                               ^^^^^^ help: a function with a similar name exists: `stat`
    |
   ::: /Users/maci/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.119/src/unix/mod.rs:715:5
    |
715 |     pub fn stat(path: *const c_char, buf: *mut stat) -> ::c_int;
    |     ------------------------------------------------------------ similarly named function `stat` defined here

With the code in the PR the built would fail, as expected, but with a much nicer error

error: failed to run custom build command for `procfs v0.12.0 (/Users/maci/github/procfs)`

Caused by:
  process didn't exit successfully: `/Users/maci/github/procfs/target/debug/build/procfs-c7cdf625ccdd8b75/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Building procfs on an for a unsupported platform. Currently only linux and android are supported', build.rs:7:5
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
eminence commented 2 years ago

Great idea, thanks! But do we need a custom build.rs script for this? Can we just add something to the top of lib.rs ?

#[cfg(not(any(
        target_os = "android",
        target_os = "linux", target_os = "l4re",
    )))]
compile_error!("Building procfs on an for a unsupported platform. Currently only linux and android are supported")
macisamuele commented 2 years ago

Using the build script ensures that the build stops immediately, while using compile_error you would not be shallowing the errors caused by the unsupported platform.

This means that the output would be sort of noisy.

   Compiling procfs v0.12.0 (/Users/maci/github/procfs)
error: Building procfs on an for a unsupported platform. Currently only linux and android are supported
  --> src/lib.rs:59:1
   |
59 | compile_error!("Building procfs on an for a unsupported platform. Currently only linux and android are supported");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0433]: failed to resolve: could not find `linux` in `os`
  --> src/process/mod.rs:66:14
   |
66 | use std::os::linux::fs::MetadataExt;
   |              ^^^^^ could not find `linux` in `os`

error[E0425]: cannot find function `stat64` in crate `libc`
   --> src/process/namespaces.rs:25:31
    |
25  |             if unsafe { libc::stat64(cstr.as_ptr(), &mut stat) } != 0 {
    |                               ^^^^^^ help: a function with a similar name exists: `stat`
    |
   ::: /Users/maci/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.119/src/unix/mod.rs:715:5
    |
715 |     pub fn stat(path: *const c_char, buf: *mut stat) -> ::c_int;
    |     ------------------------------------------------------------ similarly named function `stat` defined here

NOTE: I've updated the PR to use compile_error in the build.rs file as it provides a more visible failure output

   Compiling procfs v0.12.0 (/Users/maci/github/procfs)
error: Building procfs on an for a unsupported platform. Currently only linux and android are supported
 --> build.rs:7:5
  |
7 |     compile_error!("Building procfs on an for a unsupported platform. Currently only linux and android are supported")
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `procfs` due to previous error

@eminence : if you have a strong opinion on not having the build.rs file I can move the code into lib.rs with no sweat

eminence commented 2 years ago

I definitely agree that the error message is better when using the build.rs script. My only worry was that a build.rs script would negatively impact performance. I tried to benchmark it, but the slowdown was less than the measurement noise, so.... 🤷‍♂️

macisamuele commented 2 years ago

My only worry was that a build.rs script would negatively impact performance.

The build script is only used at compile time and it has no artifacts generates into the "production" code. The build-time will be slightly extended due to the fact that build.rs needs to be built and run before building the crate. Considering that it is essentially an empty rust binary, with no dependencies it's build and run time would be negligible.

eminence commented 2 years ago

yeah, but I already have at least 1 issue complaining about the procfs compile time ;)