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

DRAFT - POC for managing version across binaries #5469

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cipherzzz
Copy link

This pull request introduces several changes to the stacks-common and stacks-signer modules to enhance version handling and improve code organization. The most important changes include the addition of a new versions module, updates to the Cargo.toml file, and modifications to the build.rs file to handle version information more efficiently.

Enhancements to version handling:

  • stacks-common/src/util/versions.rs: Added a new versions module that includes functions to retrieve and format build versions for different binaries.
  • stacks-common/build.rs: Introduced a new Versions struct and functions to read version information from a versions.yaml file and override it with environment variables if they exist.

Code organization improvements:

Updates to stacks-signer:

  • stacks-signer/src/libsigner.rs: Replaced the manual version string construction with the new get_long_version function from the versions module.
  • stacks-signer/src/main.rs: Integrated the get_build_version function from the versions module to retrieve build version information.

Description

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@cipherzzz cipherzzz requested review from a team as code owners November 15, 2024 22:09
@cipherzzz
Copy link
Author

@wileyj @hstove

# cd stacks-signer dir
STACKS_SIGNER_VERSION=3.2.1.0 cargo build

# cd targets/debug
./stacks-signer --version

@@ -250,6 +250,11 @@ pub struct MonitorSignersArgs {
pub max_age: u64,
}

#[derive(Parser, Debug, Clone)]
Copy link
Contributor

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"
Copy link
Contributor

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

Comment on lines +28 to +31
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())
}
Copy link
Contributor

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).

Copy link
Contributor

@hstove hstove left a 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.

@hstove
Copy link
Contributor

hstove commented Nov 16, 2024

Nice work on opening your first PR btw! 👊

@cipherzzz
Copy link
Author

@hstove @wileyj - It looks like the heavy lifting is already done in #5399 . So, the remaining work is just to

  • Integrate a version to the remaining binaries
    • I assume you want to use the stacks-node version for these remaining cli versions - and keep the signer version separate?

@hstove
Copy link
Contributor

hstove commented Nov 18, 2024

@cipherzzz yep!

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.

2 participants