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

Fix: CMake and GitHub Actions confusion about what is a (stable) tag #8919

Merged
merged 2 commits into from Apr 1, 2021

Conversation

TrueBrain
Copy link
Member

Motivation / Problem

You can't have a release without things blowing up in your face :D

Description

Two mistakes:

  • GHA was looking at ISSTABLETAG to find if it started a build based on a tag. This should be ISTAG.
  • CMake set ISSTABLETAG to true for betas and RC1 but not for the stable release.

I am not 100% sure about the last one, but what we have made no sense either way. This variable is used for _openttd_newgrf_version; I guess that means we pushed betas and RCs with the indication they were stable. So at the very least we have to push 1.11.0 as being stable. So this is not a regression for sure. But I have no clue if betas or RCs should be marked as "stable" or not.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

The 6th is "is-stable-tag", but it is currently broken in meaning.
Betas and RCs are considered "stable", but final releases are not.
This is the reason it was working for RC1, but not for the final
release.
This feels a bit inside out, but it makes sense: if there are no
"beta1" or "RC1" mentions, it means it is a stable release.
@LordAro LordAro requested a review from glx22 April 1, 2021 11:09
@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Apr 1, 2021
@TrueBrain
Copy link
Member Author

TrueBrain commented Apr 1, 2021

Original code for CMake was:

		if [ -n "`echo \"${TAG}\" | grep \"^[0-9.]*$\"`" ]; then
			ISSTABLETAG="1"
		else
			ISSTABLETAG="0"
		fi

( https://github.com/OpenTTD/OpenGFX/blob/master/findversion.sh#L88-L92 )

This checks if the tag is a stable build. The grep will only return if it is, so -n means: this was a stable.

The CMake, because it replaces, is a bit harder to read: it checks if the version is a stable build; if so, it empties out the variable. So we have to check if it is empty, for it to be stable. Not as it was, non-empty. So yeah, pretty sure this PR is correctly recovering the original intention :)

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Indeed CMake test is the opposite of grep test, as we now get an empty result for tags, while with grep we wanted non empty.
And yup wrong field used in GHA.

@TrueBrain TrueBrain merged commit 5010870 into OpenTTD:master Apr 1, 2021
@TrueBrain TrueBrain deleted the fix-release branch April 1, 2021 11:26
@TrueBrain TrueBrain added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 1, 2021
LordAro added a commit to LordAro/OpenTTD that referenced this pull request Apr 1, 2021
LordAro added a commit to LordAro/OpenTTD that referenced this pull request Apr 1, 2021
LordAro added a commit to LordAro/OpenTTD that referenced this pull request Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants