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.

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

All binaries built by CD workflow are labeled as release binaries #883

Closed
hannobraun opened this issue Jul 28, 2022 · 3 comments · Fixed by #1324
Closed

All binaries built by CD workflow are labeled as release binaries #883

hannobraun opened this issue Jul 28, 2022 · 3 comments · Fixed by #1324
Labels
good first issue Good for newcomers topic: build Anything relating to the build system. type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

As of #868, we now have a --version argument that displays the version, and makes a distinction between release and development versions. Unfortunately all binaries built by the CD workflow are labeled as release binaries for the purpose of the --version flag. This leads those binaries that aren't promoted to release binaries to display a wrong version.

The way it works, is that on every push to main, the CD workflow is started, and builds binaries:
https://github.com/hannobraun/Fornjot/blob/016d5d2b2ff2dd33888c1b25d38d47c1f2c4749d/.github/workflows/cd.yml#L23-L77

The flag that marks the binaries as official release binaries is wrongly set for that whole workflow:
https://github.com/hannobraun/Fornjot/blob/016d5d2b2ff2dd33888c1b25d38d47c1f2c4749d/.github/workflows/cd.yml#L14-L16

After all binaries are built, the release job starts, and will publish a release (using those binaries), if that's what is supposed to happen. Whether it's supposed to happen, is determined by the Operator | Deduce step:
https://github.com/hannobraun/Fornjot/blob/016d5d2b2ff2dd33888c1b25d38d47c1f2c4749d/.github/workflows/cd.yml#L101-L109

At this point, the binaries have already been built.

I see two ways to address this:

  1. Move the Operator | Deduce step to a separate job that runs before the binaries are built. Only set FJ_OFFICIAL_RELEASE, if it detects that a release should happen.
  2. Don't build binaries at all, unless a release is detected. We have weekly releases now, so maybe we don't need the per-build binaries anymore?

I think I'd prefer solution 1, but I'd accept solution 2.

Labeling as https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, since this only requires knowledge of GitHub Actions to address, not any deeper understanding of Fornjot in particular.

@hannobraun hannobraun added type: bug Something isn't working good first issue Good for newcomers topic: build Anything relating to the build system. labels Jul 28, 2022
@hannobraun
Copy link
Owner Author

@hendrikmaus has left a comment in #850 with a proposal on how to approach a solution to this issue.

@kopackiw
Copy link
Contributor

kopackiw commented Nov 8, 2022

I'd like to work on that issue, but need to clarify some things first.

We have two flags:

  1. FJ_OFFICIAL_RELEASE (currently hardcoded to 1) which tells whether the release is official (e.g. production one) or not (e.g. development one).
  2. release-detected (wrapped as the Outputs::ReleaseDetected) set by the release operator (a.k.a. Operator | Deduce) which tells us whether we should trigger a release or not.

But... these two are the same flags, aren't they? If so, I've created a draft PR that unifies these two.

❗Please note, that I did not test this code against GH Actions.

@hendrikmaus
Copy link
Contributor

Thanks for the contribution @kopackiw. It is the way to go on this one. I left a review for you with a couple of comments. Let me know if there are any questions, I am happy to answer them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers topic: build Anything relating to the build system. type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants