-
Notifications
You must be signed in to change notification settings - Fork 654
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
Automatic gui version #3806
Automatic gui version #3806
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.
Thank you @andrei-toterman!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3806 +/- ##
=======================================
Coverage 88.94% 88.94%
=======================================
Files 256 256
Lines 14584 14584
=======================================
Hits 12972 12972
Misses 1612 1612 ☔ View full report in Codecov by Sentry. |
2527d75
to
5732634
Compare
5732634
to
0925844
Compare
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.
Hey Andrei. The approach looks solid. Just a couple things inline.
else() | ||
message(FATAL_ERROR "Invalid tag detected: ${NEW_VERSION}") | ||
endif() | ||
|
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 whole block could be run as a separate step, right? Could we have a separate function for it instead, and call it only where relevant (i.e. Flutter build, possibly MSI stuff later)?
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 can't be sure what you are referring to, because GitHub shows you highlighted only an empty line, but I guess you mean this whole new algorithm for extracting those two version components. And yes, we could definitely move it to a new function and call it only in the GUI cmake file.
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.
Yes, that is what I meant. Should have been clearer. BTW, I like that you are printing the vars that are getting derived. It would be nice if you could keep that.
4a313e7
to
706cc87
Compare
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.
Approving on the same grounds as on the other side.
706cc87
to
8c24637
Compare
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
Set the GUI version using the --build-name and --build-number args, using the MULTIPASS_VERSION CMake var. This way, we don't have to manually update the version all the time.
For the --build-number, the argument must only contain digits, so we extract the digits out of the commit hash respective to the current version.
MULTI-1702