-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Embed the git commit in fj-app's version number #868
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, @Michael-F-Bryan! Looks good overall, but I found some potential issues. I left comments.
@@ -11,6 +11,10 @@ env: | |||
# used to rename and upload the binaries | |||
PROJ_NAME: fj-app | |||
|
|||
# This lets our app know it's an "official" release. Otherwise we would get | |||
# a version number like "fj-app 0.8.0 (8cb928bb, unreleased)" | |||
FJ_OFFICIAL_RELEASE: 1 |
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 believe this is incorrect. The CD workflow is run (and builds binaries for) every push to main
. If a release is detected, those binaries are re-used for the actual release.
Note the Operator | Deduce
step, which detects whether we're dealing with a release, or a regular push to main
, and the if: ${{ steps.release.outputs.release-detected == 'true' }}
bits that use that step's output.
Not sure how good your grasp on GitHub Actions is. We might need @hendrikmaus to help figure this out.
If this turns into a problem, we could consider removing the per-push-to-main
build, and only build binaries for the release. I'm not sure how useful those per-push binaries are anymore, with weekly releases. Thoughts?
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.
Ahh, I just had a deeper look into how the release job works, and it seems like every push to main
generates its own compiled binaries, but they only get promoted to a proper release if the "release operator" says so.
That makes things more complicated because fj-app
won't know at compile time whether it is a proper release (the release
job depends on binaries
, but Binaries | Compile
would need to know steps.release.outputs.release-detected
- which is a dependency cycle).
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, @Michael-F-Bryan, the new commits look good!
The only remaining problem is that all CD binaries are blessed as release binaries, not just the actual release ones.
Ideas for addressing this:
- Fix this problem by cleverly changing the CD workflow.
- Fix this problem less cleverly: only build binaries for actual releases.
- Ignore the problem, merge this PR, open an issue.
The more I'm thinking about this, the more I'm leaning towards solution 3. This is already a nice improvement, and there's no need to hold it back because of one edge case.
I'll think some more. I expect to make a decision later today.
I've decided to just merge this pull request. I'll open a new issue about the wrongly labeled binaries. |
I've opened #883. |
Fixes #850.
The version number for an official build will look something like this:
Not having the
$FJ_OFFICIAL_RELEASE
variable set will addunreleased
to the version number, and having a dirty git tree will add a-modified
suffix to the commit hash.When
git
doesn't start (i.e. because it's not installed), we silently leave out the commit hash. This is a bit of an edge case, but I've found it often happens when you are building an app inside a Docker container or acargo install
from crates.io because you typically don't have access to the repo's.git/
folder.We could use something like
vergen
orbuild_info
to get more comprehensive output similar torustc --version --verbose
, but that felt like overkill and would have an impact on compile times.