-
Notifications
You must be signed in to change notification settings - Fork 45
Store the version string in a single place #181
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
base: master
Are you sure you want to change the base?
Conversation
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.
@edgar-bonet I appreciate that you're trying to help with a true problem but in my eyes this needs a fix in process and no fix in Git: The CI protects us from having these things go out of sync unnoticed already and mass replacing the number is trivial and takes below one minute, which is good for something done as rarely in my book. So I'm having trouble approving something that fixes something that should be a non-problem already with a healthy workflow and it helps feeding master
through direct pushes while it should only be fed through merged pull requests in a healthy green CI world. If the two of you want this, alright, but I consider the direct pushes to master
the problem, personally.
I wrote:
It appears this happened again on the last few version bumps. Whereas I agree with @hartwork that, in an ideal world, this should not be an issue, in practice it is annoying. Also, as the project owner, @tenox7 can choose tu push directly, without going through a pull request. We are not in a position to question this choice. It even appears that @tenox7 found that a good way to get rid of this annoyance was to simply remove UI tests step from Linux/macOS workflow. As I believe these UI tests have some value, I have rebased this pull request on top of the current master branch, with an extra commit that reverts a079775. |
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.
LGTM
I like this. Approved, if you can resolve merge conflict. Also feel free to restore the version check with this. |
This reverts commit a079775.
Instead of having the ttyplot version string in ttyplot.c (as three separate variables) and recordings/expected.txt, store it only in Makefile. This should make version bumping easier. This string is given by `make` to the compiler as a -D option. The CI workflow and get_back_in_sync.sh extract this string from the Makefile using `grep`.
I just rebased the branch on top of master and force-pushed. |
As shown by the recent version bumps (1.6.4 and 1.6.5), incrementing the version number is error prone. This is because this version number is stored in two different files (ttyplot.c and recordings/expected.txt), and these have to be kept manually in sync.
This pull request creates a single source of truth for the version string, which is the Makefile. The file expected.txt has been replaced by a template containing the string
@VERSION@
, which the CI workflow replaces by the actual version string. The script get_back_in_sync.sh does the reverse replacement.