Skip to content

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

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

Merged
hoffie merged 6 commits into from
Jun 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ecb376
Autobuild: Don't hardcode python path in analyse_git_reference.py
hoffie Apr 6, 2022
c4d9573
Autobuild: Move Changelog generation to the workflow
hoffie Apr 6, 2022
979ef53
Autobuild: Improve variable naming/coding style in analyse_git_refere…
hoffie Apr 6, 2022
abd6fa7
Autobuild: Move .github/action_scripts to .github/autobuild
hoffie Apr 6, 2022
b7745de
Autobuild: Use a variable instead of hardcoding the release changelog…
hoffie Jun 18, 2022
e97d32d
Autobuild: Avoid log output in build-vars helper function
hoffie Jun 18, 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
0 .github/actions_scripts/getChangelog.pl → .github/autobuild/extractVersionChangelog.pl
View file
Open in desktop
File renamed without changes.
28 changes: 8 additions & 20 deletions .../actions_scripts/analyse_git_reference.py → .github/autobuild/get_build_vars.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/python3
#!/usr/bin/env python3
# This script is trigged from the Github Autobuild workflow.
# It analyzes Jamulus.pro and git push details (tag vs. branch, etc.) to decide
# - whether a release should be created,
Expand Down Expand Up @@ -32,26 +32,13 @@ def get_git_hash():
]).decode('ascii').strip()


def write_changelog(version):
changelog = subprocess.check_output([
'perl',
f'{REPO_PATH}/.github/actions_scripts/getChangelog.pl',
f'{REPO_PATH}/ChangeLog',
version,
])
with open(f'{REPO_PATH}/autoLatestChangelog.md', 'wb') as f:
f.write(changelog)


def get_build_version(jamulus_pro_version):
if "dev" in jamulus_pro_version:
Copy link
Copy Markdown
Collaborator

@pljones pljones Jun 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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}")

Copy link
Copy Markdown
Member Author

@hoffie hoffie Jun 18, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@pljones pljones Jun 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@hoffie hoffie Jun 18, 2022

Choose a reason for hiding this comment

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

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?

name = "{}-{}".format(jamulus_pro_version, get_git_hash())
print("building an intermediate version: ", name)
return name
version = "{}-{}".format(jamulus_pro_version, get_git_hash())
return 'intermediate', version

name = jamulus_pro_version
print("building a release version: ", name)
return name
version = jamulus_pro_version
return 'release', version


def set_github_variable(varname, varval):
Expand All @@ -60,8 +47,9 @@ def set_github_variable(varname, varval):


jamulus_pro_version = get_version_from_jamulus_pro()
write_changelog(jamulus_pro_version)
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))
Copy link
Copy Markdown
Collaborator

@pljones pljones Jun 18, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@hoffie hoffie Jun 18, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@pljones pljones Jun 19, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@hoffie hoffie Jun 19, 2022

Choose a reason for hiding this comment

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

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

Expand Down
12 changes: 9 additions & 3 deletions .github/workflows/autobuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,21 @@ jobs:
publish_to_release: ${{ steps.get-build-vars.outputs.PUBLISH_TO_RELEASE }}
upload_url: ${{ steps.create-release.outputs.upload_url }}
build_version: ${{ steps.get-build-vars.outputs.BUILD_VERSION }}
env:
release_changelog_path: ./.github_release_changelog.md
Comment thread
ann0see marked this conversation as resolved.

steps:
- name: Checkout code
uses: actions/checkout@v2

- name: Determine release version, type and prerelease variables and generate Changelog
run: python3 ${{ github.workspace }}/.github/actions_scripts/analyse_git_reference.py
- name: Determine release version, type and prerelease variables
run: ./.github/autobuild/get_build_vars.py
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 ${{ steps.get-build-vars.outputs.JAMULUS_PRO_VERSION }} > ${{ env.release_changelog_path }}

- name: Remove release ${{steps.get-build-vars.outputs.RELEASE_TAG}}, if existing
if: steps.get-build-vars.outputs.PUBLISH_TO_RELEASE == 'true'
continue-on-error: true
Expand All @@ -90,7 +96,7 @@ jobs:
with:
tag_name: ${{ steps.get-build-vars.outputs.RELEASE_TAG }}
release_name: ${{ steps.get-build-vars.outputs.RELEASE_TITLE }}
body_path: autoLatestChangelog.md
body_path: ${{ env.release_changelog_path }}
prerelease: ${{ steps.get-build-vars.outputs.IS_PRERELEASE }}
draft: false

Expand Down
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,5 @@ distributions/jamulus-server.desktop
Debug-iphoneos/
Jamulus.xcodeproj
jamulus_plugin_import.cpp
autoLatestChangelog.md
.github_release_changelog.md
debian/