-
Notifications
You must be signed in to change notification settings - Fork 677
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
DRAFT - POC for managing version across binaries #5469
base: master
Are you sure you want to change the base?
Conversation
@@ -250,6 +250,11 @@ pub struct MonitorSignersArgs { | |||
pub max_age: u64, | |||
} | |||
|
|||
#[derive(Parser, Debug, Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this already has a --version
command in line 46
@@ -22,23 +33,34 @@ rand = { workspace = true } | |||
serde = "1" | |||
serde_derive = "1" | |||
serde_stacker = "0.1" | |||
serde_yaml = "0.9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need serde_yaml
outside of build_dependencies
pub fn get_long_version(binary: &str) -> String { | ||
let build_version = get_build_version(binary); | ||
format!("{} ({}, {}, {})", build_version, get_target_build_type(), get_target_os(), get_target_arch()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the version_string
util, which includes the arch, build type, and also git info. I think we should keep that, as it's also a part of our CI process (ie setting the git info via ENV).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, you should rebase this on top off #5399. That PR uses a "proper" build script to generate static types from a versions file, which is better / more performant than parsing YAML at runtime. Then, this PR can be just about adding a --version
flag to our existing CLIs.
Additionally, I don't think we should be tracking individual versions other than the stacks node version and signer version. We've never made separate releases for other binaries (like stacks-inspect
), and I don't suspect we'll have reason to do so in the future.
Nice work on opening your first PR btw! 👊 |
@cipherzzz yep! |
This pull request introduces several changes to the
stacks-common
andstacks-signer
modules to enhance version handling and improve code organization. The most important changes include the addition of a newversions
module, updates to theCargo.toml
file, and modifications to thebuild.rs
file to handle version information more efficiently.Enhancements to version handling:
stacks-common/src/util/versions.rs
: Added a newversions
module that includes functions to retrieve and format build versions for different binaries.stacks-common/build.rs
: Introduced a newVersions
struct and functions to read version information from aversions.yaml
file and override it with environment variables if they exist.Code organization improvements:
stacks-common/Cargo.toml
: Reformatted theauthors
andkeywords
fields for better readability and added new dependencies forserde
andserde_yaml
. [1] [2] [3]stacks-common/src/util/mod.rs
: Added the newly createdversions
module to the list of public modules.Updates to
stacks-signer
:stacks-signer/src/libsigner.rs
: Replaced the manual version string construction with the newget_long_version
function from theversions
module.stacks-signer/src/main.rs
: Integrated theget_build_version
function from theversions
module to retrieve build version information.Description
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/rpc/openapi.yaml
andrpc-endpoints.md
for v2 endpoints,event-dispatcher.md
for new events)clarity-benchmarking
repobitcoin-tests.yml