-
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
Add default version to version_string
function
#5399
base: develop
Are you sure you want to change the base?
Conversation
this does add a little extra burden. I'd like to first see if there are
other options vs what you're doing here (which seems to work fine, but may
become a bit messy with several binaries being built).
…On Tue, Oct 29, 2024 at 10:16 Hank Stoever ***@***.***> wrote:
@hstove <https://github.com/hstove> requested review from
@stacks-network/blockchain-team on: #5399
<#5399> Add default
version to version_string function as a code owner.
—
Reply to this email directly, view it on GitHub
<#5399 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVXIHA7GT7EOEWQPDPE6TLZ567IHAVCNFSM6AAAAABQ2LJ5TSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUHE4DQNRRGYYTENQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Yeah I'm totally open to other ideas, but at least this only means we update the version in two total places. If we went with the |
What if, instead of updating several source files, we used either a common file or even a text/yml file? the idea i had was to keep version constants here: |
@wileyj I've updated this PR to have a top-level |
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.
This looks good, just had 2 questions. Also, is there a good way to write a test for this?
@obycode regarding "can we test this" - there is this old Rust issue marked "closed as needs RFC" regarding writing tests for a build script. What we would have to do is either:
|
This PR updates the
version_string
function, along with our release process docs, to add a "hard-coded" version. When theSTACKS_NODE_VERSION
environment variable is not present (such as when building from source), the resulting version string will contain this default value.I tried using the
Cargo.toml
version, which is a cleaner solution, butcargo
is strict about the version format and doesn't allow our versioning scheme.