Unify release flags and correctly mark dev build releases #1324

kopackiw · 2022-11-08T01:15:08Z

❗NOT TESTED❗

This PR unifies the FJ_OFFICIAL_RELEASE and release-detected flags that carry the same meaning.

To properly read the release-detected as env, work in #1270 is required I think.

Closes #883

kopackiw · 2022-11-08T01:16:29Z

crates/fj/build.rs

@@ -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");


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

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 # ...

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.

hendrikmaus · 2022-11-08T07:58:30Z

.github/workflows/cd.yml

 defaults:
  run:
    shell: bash

 jobs:
+  calculate-release-flags:


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.

Good catch, fixed 👍🏻

hendrikmaus · 2022-11-08T07:59:35Z

.github/workflows/cd.yml

@@ -69,6 +88,7 @@ jobs:

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


Suggested change

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

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

Fixed, thank you!

hendrikmaus · 2022-11-08T08:05:25Z

crates/fj/build.rs

@@ -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");


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 # ...

hendrikmaus · 2022-11-08T08:06:39Z

.github/workflows/cd.yml

@@ -69,6 +88,7 @@ jobs:

  release:
    name: Release
+    if: ${{ steps.calculate-release-flags.outputs.release-detected == 'true' }}
    needs: binaries


Suggested change

needs: binaries

needs:

- binaries

- calculate-release-flags

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!

hannobraun

Thank you for this pull request, @kopackiw!

This looks good to me (modulo the problem with the output/variable that @hendrikmaus pointed out). Regarding testing, I assume it would be a pain to set up a test outside of this repository, so let's just merge it and see how it goes, once @hendrikmaus runs out of problems to point out 😄

hannobraun · 2022-11-08T09:42:42Z

crates/fj/build.rs

@@ -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");


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.

kopackiw · 2022-11-08T14:36:21Z

Thank you both for a detailed review! I've fixed all issues and I think we're ready to go out from a draft to a mergeable PR. I think next step is to test it on production 😄

hannobraun

Thank you for the changes, @kopackiw!

As far as I'm concerned, this is good to go, but I'd like to give @hendrikmaus the chance to take another look. I'll merge as soon as he gives the go-ahead!

hendrikmaus

Thanks @hannobraun, I think we're good to go.

hannobraun · 2022-11-09T08:24:13Z

Thanks, @hendrikmaus! Merging.

hannobraun · 2022-11-09T08:34:24Z

Everything seems to work perfectly! Binaries are built, no errors, and --version correctly indicates that it is not a release. Thanks again, @kopackiw and @hendrikmaus!

kopackiw added 4 commits November 8, 2022 02:05

Move release operator into separate job

7175115

Move "if" checks into job rather than separate steps

cecbd97

Replace the "FJ_OFFICIAL_RELEASE" flag with the "release-detected"

a398b43

Compiling binaries step now requires calculated release flag step

3a1b25c

kopackiw commented Nov 8, 2022

View reviewed changes

hendrikmaus suggested changes Nov 8, 2022

View reviewed changes

hendrikmaus reviewed Nov 8, 2022

View reviewed changes

hannobraun reviewed Nov 8, 2022

View reviewed changes

kopackiw added 5 commits November 8, 2022 15:09

Merge branch 'main' into issue-883

755dab2

Set outputs for the calculate-release-flags job

9759d92

Change "steps" to "needs" in the release if expression

dade305

Add calculate-release-flags as dependecy for release

60b4992

Pass RELEASE_DETECTED as env for a compilation step

639e1f1

kopackiw marked this pull request as ready for review November 8, 2022 14:33

kopackiw added 2 commits November 8, 2022 15:37

Fix step name

084bad5

Bring back removed comment for clarity with new env

adcef14

hannobraun approved these changes Nov 8, 2022

View reviewed changes

hendrikmaus approved these changes Nov 9, 2022

View reviewed changes

hannobraun merged commit fed6533 into hannobraun:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify release flags and correctly mark dev build releases #1324

Unify release flags and correctly mark dev build releases #1324

kopackiw commented Nov 8, 2022 •

edited

Loading

kopackiw Nov 8, 2022

hendrikmaus Nov 8, 2022

hannobraun Nov 8, 2022

hendrikmaus Nov 8, 2022

kopackiw Nov 8, 2022

hendrikmaus Nov 8, 2022

kopackiw Nov 8, 2022

hendrikmaus Nov 8, 2022

hendrikmaus Nov 8, 2022 •

edited

Loading

kopackiw Nov 8, 2022

hannobraun left a comment

hannobraun Nov 8, 2022

kopackiw commented Nov 8, 2022

hannobraun left a comment

hendrikmaus left a comment

hannobraun commented Nov 9, 2022

hannobraun commented Nov 9, 2022

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

Unify release flags and correctly mark dev build releases #1324

Unify release flags and correctly mark dev build releases #1324

Conversation

kopackiw commented Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendrikmaus Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannobraun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopackiw commented Nov 8, 2022

hannobraun left a comment

Choose a reason for hiding this comment

hendrikmaus left a comment

Choose a reason for hiding this comment

hannobraun commented Nov 9, 2022

hannobraun commented Nov 9, 2022

kopackiw commented Nov 8, 2022 •

edited

Loading

hendrikmaus Nov 8, 2022 •

edited

Loading