refactor: migrate VersionInfo to builder binary #473
Conversation
Move VersionInfo struct and register_version_metrics from op-rbuilder crate to the builder binary. This decouples version metrics registration from the crate, allowing binaries to control their own version reporting. - Add bin/builder/src/version.rs with VersionInfo and metrics registration - Update launcher to accept on_node_started callback - Move SHORT_VERSION/LONG_VERSION constants to args/mod.rs - Simplify op-rbuilder build.rs to only generate CLI version env vars - Remove VersionInfo from metrics.rs Closes base#396
🟡 Heimdall Review Status
|
haardikk21
requested review from
haardikk21 and
refcell
and removed request for
refcell
| pub const SHORT_VERSION: &str = env!("OP_RBUILDER_SHORT_VERSION"); | ||
|
|
||
| /// Long version string with additional build info. | ||
| pub const LONG_VERSION: &str = concat!( | ||
| env!("OP_RBUILDER_LONG_VERSION_0"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_1"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_2"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_3"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_4"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_5"), | ||
| ); |
There was a problem hiding this comment.
Why are these duplicated?
| if let Err(err) = op_rbuilder::launcher::launch(|| { | ||
| version::VERSION.register_version_metrics(); | ||
| }) { |
There was a problem hiding this comment.
Why do we need a callback function here instead of simply calling the function after calling launch?
| pub const SHORT_VERSION: &str = env!("OP_RBUILDER_SHORT_VERSION"); | ||
|
|
||
| /// Long version string with additional build info. | ||
| pub const LONG_VERSION: &str = concat!( | ||
| env!("OP_RBUILDER_LONG_VERSION_0"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_1"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_2"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_3"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_4"), | ||
| "\n", | ||
| env!("OP_RBUILDER_LONG_VERSION_5"), | ||
| ); | ||
|
|
There was a problem hiding this comment.
all these OP_RBUILDER env vars will not be available in this context due to missing code migrations from the build.rs file of the crate they previously belong to
| println!("cargo:rustc-env=OP_RBUILDER_BUILD_PROFILE={profile}"); | ||
|
|
||
| // Set formatted version strings | ||
| // Set formatted version strings used by the CLI | ||
| let pkg_version = env!("CARGO_PKG_VERSION"); | ||
|
|
||
| // The short version information for op-rbuilder. | ||
| // - The latest version from Cargo.toml |
There was a problem hiding this comment.
This file's diff is bit of a mess
- It gets rid of env var setting for
OP_RBUILDER_BUILD_PROFILEbut keeps several otherOP_RBUILDER_*env vars in here - A decent chunk of this logic needs to move to the new place where these env vars are loaded to set the version info. Without this, those env vars will be unset
|
Looking at @haardikk21's comments and the current layout of the builder crate, it may be worth sidelining this work for now. There's still a bunch of cleanup that needs to happen to the builder crate - especially wrt the tester binary (can we delete?). I'd recommend closing this PR for now and returning to this work later. |
should I apply the changes @haardikk21 ? |
Hold off on it for now I guess. Let's wait for the dust to settle a bit and can do it in a couple days or so |
okay then. lets wait |
4 participants
Summary
Move VersionInfo struct and register_version_metrics from op-rbuilder crate to the builder binary. This decouples version metrics registration from the crate, allowing binaries to control their own version reporting.
Changes
Closes #396