Autobuild: Improve and reorganize git/Changelog scripts (re-submission of #2584) #2656

hoffie · 2022-06-18T15:54:59Z

Short description of changes
#2584 was already supposed and documented to contain these changes, but didn't (probably due to a bad rebase). Therefore, I'm re-submitting it and will edit the old PR to match what was actually merged.

Autobuild: Don't hardcode python path in analyse_git_reference.py
Autobuild: Move Changelog generation to the workflow
Previously, the autobuild changelog (autoLatestChangelog.md) which is used for the Github release body, was generated as part of the "analyse_git_reference.py" script. It does not seem logical for an analysis script to have a side effect of producing a Changelog file. Also, the file name (autoLatestChangelog.md) was hardcoded in two places and had to match (workflow + script).

With this commit, the changelog generation call (.github/actions_scripts/getChangelog.pl) is moved to the workflow. This follows the principle of least surprise and moves the output file name usage to a single file (the workflow).

Fixes: Autobuild: Make Changelog generation an extra step #2480
Autobuild: Improve variable naming/coding style in analyse_git_reference.py
Autobuild: Move .github/action_scripts to .github/autobuild
This also renames the two scripts to better reflect what they do.
Autobuild: Use a variable instead of hardcoding the release changelog path twice
Related: Autobuild: Make Changelog generation an extra step #2480

CHANGELOG: Autobuild: Improved and reorganized git/Changelog scripts
(to be condensed with the other entries)

Context: Fixes an issue?
Fixes: #2480

Does this change need documentation? What needs to be documented and how?
No.

Status of this Pull Request
Ready for review.

What is missing until this pull request can be merged?

Reviews.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want: Yes, on my fork: https://github.com/hoffie/jamulus/runs/6950719696?check_suite_focus=true#step:4:5 https://github.com/hoffie/jamulus/releases/tag/r0_0_1
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

pljones · 2022-06-18T16:07:27Z

.github/autobuild/get_build_vars.py

-
-def get_build_version(jamulus_pro_version):
+def get_build_version():
+    jamulus_pro_version = get_version_from_jamulus_pro()
    if "dev" in jamulus_pro_version:


I'm not keen on logging in this function. I'd rather it returned a tuple of the type and version and the caller could choose to log or not.

def get_build_type_version(): jamulus_pro_version = get_version_from_jamulus_pro() type = "release" version = jamulus_pro_version if "dev" in jamulus_pro_version: type = "dev" version = "{}-{}".format(version, get_git_hash()) return type, version

build_type, build_version = get_build_type_version() print(f"building a {build_type} version: {build_version}")

I don't feel strongly about that and I think I've just taken what was there.
I have now (mostly) applied your suggestion.

Yep wasn't fussed as it was already there. But as it was potentially useful information (i.e. build type), it might as well be exposed to the caller.

On which note - is the check for dev here the consistent check now?

On which note - is the check for dev here the consistent check now?

Sorry, I missed that part -- Github doesn't send notifications for edits.
Can you elaborate on what you mean here?

ann0see · 2022-06-18T20:27:10Z

.github/workflows/autobuild.yml

        id:                         get-build-vars

+      - name:                       Extract Changelog for the Github release body
+        if:                         steps.get-build-vars.outputs.PUBLISH_TO_RELEASE == 'true'
+        run:                        ./.github/autobuild/extractVersionChangelog.pl ChangeLog $(grep -oP 'VERSION = \K\w[^\s\\]*' Jamulus.pro) > ${{ env.release_changelog_path }}


Not that happy with a regex and grep of the version here. Don't we have the version somewhere else?

The very version we need here is available in the script, but currently isn't output. We can change that, but it would be a more distributed change. See here:
https://github.com/jamulussoftware/jamulus/pull/2655/files#diff-54b7cee412550302a3e076162e611b9719407b264c79553452499450c54a07e5R52 https://github.com/jamulussoftware/jamulus/pull/2655/files#diff-55d631a22c9e350832273ee0abf8a6013435fa6051789cbe311b981b2ec0f348R88

Would you prefer that?

That's rather why I was wondering if returning the release type from get_version_from_jamulus_pro() might help elsewhere.

Yes, personally, I'd prefer this since it feels cleaner.

Ok, updated and re-tested on my fork (updated the link in the PR description).

Previously, the autobuild changelog (autoLatestChangelog.md) which is used for the Github release body, was generated as part of the "analyse_git_reference.py" script. It does not seem logical for an analysis script to have a side effect of producing a Changelog file. Also, the file name (autoLatestChangelog.md) was hardcoded in two places and had to match (workflow + script). With this commit, the changelog generation call (.github/actions_scripts/getChangelog.pl) is moved to the workflow. This follows the principle of least surprise and moves the output file name usage to a single file (the workflow). Fixes: jamulussoftware#2480

…nce.py

ann0see

Thanks. Just another comment:

.github/workflows/autobuild.yml

This also renames the two scripts to better reflect what they do.

… path twice Related: jamulussoftware#2480

Related: https://github.com/jamulussoftware/jamulus/pull/2656/files/42e085c2254984162ebf964154ca8aa4041a9046#r900988442

pljones · 2022-06-18T21:20:01Z

.github/autobuild/get_build_vars.py

-build_version = get_build_version(jamulus_pro_version)
+set_github_variable("JAMULUS_PRO_VERSION", jamulus_pro_version)
+build_type, build_version = get_build_version(jamulus_pro_version)
+print(f'building a version of type "{build_type}": {build_version}')

 fullref = os.environ['GITHUB_REF']
 publish_to_release = bool(re.match(r'^refs/tags/r\d+_\d+_\d+\S*$', fullref))


OK, can we either do this or do what line 36 (if "dev" in jamulus_pro_version:) does, rather than have both ways in use.

I guess, ideally that means another function get_release_type() that just returns whether this is a release or not. It can be boolean, to allow the caller to decide how to use the result - but if we're using the tag (as here) to drive what gets triggered in the automation, then the scripts (that get triggered) should also just use the tag.

So

do we need to differentiate between GITHUB_REF version and Jamulus.pro version?

if so, we should be consistent and use GITHUB_REF at least to drive whether a build is a release or not

if not, we should be completely consistent and use one or other - everywhere

OK, can we either do this or do what line 36 (if "dev" in jamulus_pro_version:) does, rather than have both ways in use.

I don't quite see yet how one could replace the other.

I guess, ideally that means another function get_release_type() that just returns whether this is a release or not. It can be boolean, to allow the caller to decide how to use the result

That would be possible, for sure.

but if we're using the tag (as here) to drive what gets triggered in the automation, then the scripts (that get triggered) should also just use the tag.

do we need to differentiate between GITHUB_REF version and Jamulus.pro version?

With the current design, assumptions and constraints, we have to differentiate, I think. GITHUB_REF(= tag) might be different (e.g. for pre-releases) or not existing at all (branches/master).

if so, we should be consistent and use GITHUB_REF at least to drive whether a build is a release or not

I think this should be the case already. We use the workflow variable PUBLISH_TO_RELEASE to drive the build. This variable is solely driven by GITHUB_REF.

if not, we should be completely consistent and use one or other - everywhere

I think there's certainly room for improvement, but it needs thorough checking to avoid breaking existing use cases (such as local builds). In addition, I don't think this PR introduces any additional inconsistencies. It might make existing strangeness more obvious (which is a good thing, IMO). Therefore, I'm a bit hesitant to squeeze in additional larger changes here.

My concern is get_build_version() uses a different value. The tag, Jamulus.pro and ChangeLog could all have different versions.

Given we have three sources for the version, we really need automation to make sure they're aligned, at least: i.e. error out if things look wrong. But I agree, that's not in scope here. For that, it sounds like (and I don't fully understand how this works):

If the tag is set, we should use that to determine build type and version and that source should overrule others.

ChangeLog and Jamulus.pro "target" versions should match - and, if set, match the tag build type and version.

If the tag wasn't set, build type and version can then come from ChangeLog and Jamulus.pro, as needed.

Yep. I've split that out into #2662 to track that.

Autobuild: Don't hardcode python path in analyse_git_reference.py

5ecb376

hoffie added this to the Release 3.9.0 milestone Jun 18, 2022

hoffie mentioned this pull request Jun 18, 2022

Autobuild: Clean up redundant variables in workflow definition #2584

Merged

5 tasks

hoffie mentioned this pull request Jun 18, 2022

Autobuild: Combine scripts into single script per-platform #2503

Closed

22 tasks

pljones reviewed Jun 18, 2022

View reviewed changes

pljones approved these changes Jun 18, 2022

View reviewed changes

hoffie requested a review from ann0see June 18, 2022 17:35

ann0see reviewed Jun 18, 2022

View reviewed changes

hoffie added 2 commits June 18, 2022 22:50

Autobuild: Improve variable naming/coding style in analyse_git_refere…

979ef53

…nce.py

hoffie force-pushed the autobuild-action-scripts-redone branch from 4df0df3 to 9060f2c Compare June 18, 2022 20:57

ann0see reviewed Jun 18, 2022

View reviewed changes

.github/workflows/autobuild.yml Show resolved Hide resolved

Autobuild: Move .github/action_scripts to .github/autobuild

abd6fa7

This also renames the two scripts to better reflect what they do.

hoffie force-pushed the autobuild-action-scripts-redone branch from 9060f2c to 1d10c31 Compare June 18, 2022 21:03

hoffie added 2 commits June 18, 2022 23:09

Autobuild: Use a variable instead of hardcoding the release changelog…

b7745de

… path twice Related: jamulussoftware#2480

Autobuild: Avoid log output in build-vars helper function

e97d32d

Related: https://github.com/jamulussoftware/jamulus/pull/2656/files/42e085c2254984162ebf964154ca8aa4041a9046#r900988442

hoffie force-pushed the autobuild-action-scripts-redone branch from 1d10c31 to e97d32d Compare June 18, 2022 21:09

pljones reviewed Jun 18, 2022

View reviewed changes

pljones approved these changes Jun 19, 2022

View reviewed changes

hoffie mentioned this pull request Jun 19, 2022

Build: Version comes from git tag, ChangeLog, Jamulus.pro #2662

Open

hoffie requested a review from ann0see June 19, 2022 10:26

ann0see approved these changes Jun 19, 2022

View reviewed changes

hoffie merged commit 42916f7 into jamulussoftware:master Jun 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autobuild: Improve and reorganize git/Changelog scripts (re-submission of #2584) #2656

Autobuild: Improve and reorganize git/Changelog scripts (re-submission of #2584) #2656

hoffie commented Jun 18, 2022 •

edited

Loading

pljones Jun 18, 2022 •

edited

Loading

hoffie Jun 18, 2022

pljones Jun 18, 2022 •

edited

Loading

hoffie Jun 18, 2022

ann0see Jun 18, 2022

hoffie Jun 18, 2022

pljones Jun 18, 2022

ann0see Jun 18, 2022

hoffie Jun 18, 2022

ann0see left a comment

pljones Jun 18, 2022

hoffie Jun 18, 2022

pljones Jun 19, 2022

hoffie Jun 19, 2022

Autobuild: Improve and reorganize git/Changelog scripts (re-submission of #2584) #2656

Autobuild: Improve and reorganize git/Changelog scripts (re-submission of #2584) #2656

Conversation

hoffie commented Jun 18, 2022 • edited Loading

Checklist

pljones Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Jun 18, 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

ann0see left a 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

hoffie commented Jun 18, 2022 •

edited

Loading

pljones Jun 18, 2022 •

edited

Loading

pljones Jun 18, 2022 •

edited

Loading