autometrics-rs icon indicating copy to clipboard operation
autometrics-rs copied to clipboard

Autometrics doesn't compile with Bazel due to `CARGO_PKG_REPOSITORY` not defined at compile time

Open marvin-hansen opened this issue 1 year ago • 3 comments

I've a larger mono-repo that builds with Bazel and when adding autometrics as dependency, compile fails with the error below.

Apparently, the issue is caused because the repo_url in the setting file cannot access the environment variable. Unlike Cargo, Bazel operates in a hermetic environment meaning Bazel cannot access host environment variables so this is actually expected behavior for hermetic builds.

The file in question: https://github.com/autometrics-dev/autometrics-rs/blob/9bc24e0935103c990ff5a963079c5db6c4a4792f/autometrics/src/settings.rs#L201

I had similar issues in the past and, by experience, relying on settings stored in an env variables that is expected on the host was almost always problematic, so there are roughly three ways to resolve this issue:

  1. Ensure repo url is initialized during construction. The RAII pattern does makes sense here unless the URL somehow isn't invariant.

  2. Modify the recovery strategy. Currently, the implementation tries to read the env variable like so:

     let repo_url = self
            .repo_url
            .or_else(|| env::var("AUTOMETRICS_REPOSITORY_URL").ok())
            .unwrap_or_else(|| env!("CARGO_PKG_REPOSITORY").to_string());

The problem is that repo_url isn't initialized, but the env variable is inaccessible either from within Bazel. Therefore, if the repo_url is invariant, you can store it in a constant and use that as a fallback. If it's not invariant, for whatever reason, you would need to add a small util that constructs the correct repo_url.

  1. The error message suggests to switch to "std::env::var" to read env variables generated at runtime. Give it a try and see if this fixes anything, although I don't think that it resolves the problem that the autometrics repo url is still not initialized prior to accessing it.

Personally, in my project, I rely on configuration as code and only use one single env variable "ENV" to determine in which environment the system runs in order to determine which configuration to load.

I don't necessarily want to write a patch & fill a PR at this time because, quite frankly, I have a hard time understanding how settings configuration works in autometrics let alone how cargo config works in this context. However, If necessary, I can provide a self-contained code example with some instructions how to run the Bazel build if it helps to get this fixed.

The full error log:

 exec env - \
    CARGO_CFG_TARGET_ARCH=aarch64 \
    CARGO_CFG_TARGET_OS=darwin \
    CARGO_CRATE_NAME=autometrics \
    CARGO_MANIFEST_DIR='${pwd}/external/crate_index__autometrics-1.0.1' \
    CARGO_PKG_AUTHORS='' \
    CARGO_PKG_DESCRIPTION='' \
    CARGO_PKG_HOMEPAGE='' \
    CARGO_PKG_NAME=autometrics \
    CARGO_PKG_VERSION=1.0.1 \
    CARGO_PKG_VERSION_MAJOR=1 \
    CARGO_PKG_VERSION_MINOR=0 \
    CARGO_PKG_VERSION_PATCH=1 \
    CARGO_PKG_VERSION_PRE='' \
    OUT_DIR='${pwd}/bazel-out/darwin_arm64-fastbuild/bin/external/crate_index__autometrics-1.0.1/autometrics_bs.out_dir' \
... 
# Execution platform: @@local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
error: environment variable `CARGO_PKG_REPOSITORY` not defined at compile time
   --> external/crate_index__autometrics-1.0.1/src/settings.rs:204:32
    |
204 |             .unwrap_or_else(|| env!("CARGO_PKG_REPOSITORY").to_string());
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: Cargo sets build script variables at run time. Use `std::env::var("CARGO_PKG_REPOSITORY")` instead
    = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 1 previous error

marvin-hansen avatar May 10 '24 07:05 marvin-hansen

Hi, thanks for the issue! We talked internally and decided that we'll change the code to instead use env_optional! with a empty string as a fallback. In such a situation however, we recommend calling the AutometricsSettingsBuilder to set a repo_url for usage during runtime:

fn main() {
    AutometricsSettings::builder()
        .repo_url("https://github.com/autometrics-dev/autometrics-rs/")
        .init();
    
    // ...
}

In the meantime while we add a fallback, I suggest as a workaround that you define CARGO_PKG_REPOSITORY (can also just be an empty string) in your Bazel build.

mellowagain avatar May 13 '24 12:05 mellowagain

For the record, the reason we thought it was fine was the Cargo book:

Cargo exposes these environment variables to your crate when it is compiled

That’s why we were kind of expecting the variables to be always accessible if cargo is used to compile, even for hermetic builds. The more we know

gagbo avatar May 13 '24 12:05 gagbo

Thanks for working on this issue, I sincerely appreciate your efforts.

For clarification, the cargo book is correct. However Bazel invokes the Rust compiler directly meaning whatever magic cargo may cook up along the way is not working when Bazel executes the build.

I try the environment variable as you said and report back probably tomorrow when I'm back in office.

Again thank you so much for ironing this out because this also resolves any potential issues with buck2 and all other enterprise grade build systems.

On Mon, May 13, 2024 at 8:06 PM Gerry Agbobada @.***> wrote:

For the record, the reason we thought it was fine was the Cargo book https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates :

Cargo exposes these environment variables to your crate when it is compiled

That’s why we were kind of expecting the variables to be always accessible if cargo is used to compile, even for hermetic builds. The more we know

— Reply to this email directly, view it on GitHub https://github.com/autometrics-dev/autometrics-rs/issues/176#issuecomment-2107400274, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFYR7XG47772XKQ3MF4BUKTZCCUFXAVCNFSM6AAAAABHQE7VB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBXGQYDAMRXGQ . You are receiving this because you authored the thread.Message ID: @.***>

marvin-hansen avatar May 13 '24 12:05 marvin-hansen

Closing this issue as its currently impossible to compile autometrics with Bazel.

After having worked around the aforementioned environment variable issue, I was running into 99 errors related to conflicting feature flags and versions. It's not exactly clear to me how that is even possible because the crates_repository index in Bazel usually resolves all feature flags correctly so I am really out of my depth here.

However, as a word of warning, whenever you change build environment variables in Bazel, you actually invalidate the cache and trigger a full rebuild plus you have to carry over those variables to test and run targets meaning if your repo is large, you spent the rest of the day watching the compiler....

That is to say, if this ever get touched again, please add a custom constructor and pass in the repo_url as argument, as shown in the example above, to prevent any reliance on env variables as these only cause 99 unnecessary problems at scale.

it's a pity because this is an outstanding crate that adds real value.

marvin-hansen avatar May 23 '24 06:05 marvin-hansen