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.

Sign up for GitHub

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

Unify release flags and correctly mark dev build releases #1324

Merged
merged 11 commits into from
Nov 9, 2022
Merged

Unify release flags and correctly mark dev build releases #1324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7175115
Move release operator into separate job
kopackiw Nov 8, 2022
cecbd97
Move "if" checks into job rather than separate steps
kopackiw Nov 8, 2022
a398b43
Replace the "FJ_OFFICIAL_RELEASE" flag with the "release-detected"
kopackiw Nov 8, 2022
3a1b25c
Compiling binaries step now requires calculated release flag step
kopackiw Nov 8, 2022
755dab2
Merge branch 'main' into issue-883
kopackiw Nov 8, 2022
9759d92
Set outputs for the `calculate-release-flags` job
kopackiw Nov 8, 2022
dade305
Change "steps" to "needs" in the `release` if expression
kopackiw Nov 8, 2022
60b4992
Add `calculate-release-flags` as dependecy for `release`
kopackiw Nov 8, 2022
639e1f1
Pass `RELEASE_DETECTED` as env for a compilation step
kopackiw Nov 8, 2022
084bad5
Fix step name
kopackiw Nov 8, 2022
adcef14
Bring back removed comment for clarity with new env
kopackiw Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions .github/workflows/cd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,36 @@ 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

defaults:
run:
shell: bash

jobs:
calculate-release-flags:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, this is the way to go.
Minor thing to fix: the job needs to define the output that the release operator populates, see:
https://docs.github.com/en/actions/using-jobs/defining-outputs-for-jobs

Like that, the subsequent jobs can access the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed 👍🏻

name: Calculate release flags
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Operator | Cache
uses: Swatinem/rust-cache@v2
with:
key: release-operator-01

- name: Operator | Deduce
id: release
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
RELEASE_LABEL: release
RUST_LOG: info
run: |
# Run release operator
cargo run -p release-operator -- detect

binaries:
name: Binaries
needs: calculate-release-flags
strategy:
matrix:
include:
Expand Down Expand Up @@ -69,6 +88,7 @@ jobs:

release:
name: Release
if: ${{ steps.calculate-release-flags.outputs.release-detected == 'true' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ steps.calculate-release-flags.outputs.release-detected == 'true' }}
if: ${{ needs.calculate-release-flags.outputs.release-detected == 'true' }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thank you!

needs: binaries
Copy link
Contributor

@hendrikmaus hendrikmaus Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
needs: binaries
needs:
- binaries
- calculate-release-flags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote that way because the calculate-release-flags is an indirect dependency via the binaries. But it won't work with an if expression above: if: ${{ needs.calculate-release-flags.outputs.release-detected == 'true' }}, so fixed as well!

runs-on: ubuntu-latest
steps:
Expand All @@ -80,22 +100,10 @@ jobs:
with:
key: release-operator-01

- name: Operator | Deduce
id: release
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
RELEASE_LABEL: release
RUST_LOG: info
run: |
# Run release operator
cargo run -p release-operator -- detect

- name: Binaries | Download
if: ${{ steps.release.outputs.release-detected == 'true' }}
uses: actions/download-artifact@v3

- name: Binaries | Checksums
if: ${{ steps.release.outputs.release-detected == 'true' }}
run: |
# Build binary checksums
for file in "${PROJ_NAME}"-*/"${PROJ_NAME}"-*; do
Expand All @@ -105,15 +113,13 @@ jobs:
done

- name: Release | GitHub
if: ${{ steps.release.outputs.release-detected == 'true' }}
uses: softprops/action-gh-release@v1
with:
tag_name: ${{ steps.release.outputs.tag-name }}
name: ${{ steps.release.outputs.tag-name }}
files: ${{ env.PROJ_NAME }}-*/${{ env.PROJ_NAME }}-*

- name: Release | Crates.io
if: ${{ steps.release.outputs.release-detected == 'true' }}
env:
RUST_LOG: info
run: |
Expand Down
4 changes: 2 additions & 2 deletions crates/fj/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ impl Version {
let commit = git_description();

let official_release =
std::env::var("FJ_OFFICIAL_RELEASE").as_deref() == Ok("1");
println!("cargo:rerun-if-env-changed=FJ_OFFICIAL_RELEASE");
std::env::var("release-detected").as_deref() == Ok("true");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name can be changed, as for me it is a bit misleading. I propose e.g. is-official-release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release operator doesn't set any environment variables, it sets github actions outputs.
So if you want the build.rs to have access to the data, you need to export it in the step that will call cargo build.
At a glance, I'd say say this needs to happen here https://github.com/hannobraun/Fornjot/pull/1324/files#diff-ea3ea8c9932adc7ba8161ceda844fedd43b006848ef1140c050cbd7ea0788a18R65

Environment variables, as a best practice, are always written in all caps. So I would propose to call it RELEASE_DETECTED then.

On the aforementioned step - the one that runs cargo build - we could then populate the variable like so:

- name: Some step
  env:
    RELEASE_DETECTED: needs.calculate-release-flags.outputs.release-detected
  # ...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to renaming this output/variable, although I'm not sure about is-official-release. There's no such thing as an unofficial release, so what exactly does that mean? (Yes, I realize the previous environment variable did the same thing.) I agree with @hendrikmaus' point about capitalization.

Personally, I'd be fine with RELEASE_DETECTED, but that's just bikeshedding. I'd merge this pull request with any semi-reasonable naming choice. We can always improve things in follow-up PRs.

println!("cargo:rerun-if-env-changed=release-detected");

let full_string = match (commit, official_release) {
(Some(commit), true) => format!("{pkg_version} ({commit})"),
Expand Down