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

Autobuild: Derive all step commands from a base_command + multiple minor fixes #2540

Merged
merged 14 commits into from
Mar 23, 2022

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Mar 23, 2022

Short description of changes

This is the last bigger PR in this Autobuild cleanup series. It performs several refactorings, cleanups and fixes which only became possible after the previous ground-work or only became apparent by now.
This PR touches multiple aspects of Autobuild, but is solely focused on refactoring without any changes in the output.

  • Autobuild: Derive all step commands from a base_command
    Previously, the build matrix had to provide three different command lines for the three different build steps. As all command invocations are normalized across platforms now, we can condense that into a base_command per matrix entry and will just add the build stage as a parameter in the generic invocation. This allows removal of several lines of definitions and makes the
    workflow more readable/maintenable.

  • Autobuild: Drop irregular contains() usage from Workflow
    The Workflow used the contains() function in multiple places. contains() will either perform an in-array check or a substring check. We are not dealing with arrays, nor do we want a substring check when checking for 'true'. Therefore, use an ordinary string comparison here which is more readable anyways.

  • Autobuild: Drop jamulus_project_path from Workflow
    The previous refactorings have adjusted all platform autobuild scripts to stop using this variable. Therefore, we can now safely drop the definition from the Workflow.

  • Autobuild: Mac: Set PATH before build step directly
    There's no need to set PATH in the prepare step, pass it back to the workflow, let the workflow pass it back to the build stage and use it only then. Therefore, just set PATH right before it's needed in the build step and drop the Github logic.

  • Autobuild: Workflow: Drop discouraged ACTIONS_ALLOW_UNSECURE_COMMANDS
    As no autobuild scripts require setting environment variables across build steps anymore, we can now safely drop this discouraged configuration.
    In case we ever need to set environment variables across steps again, there are better ways to do that by now (env files).

  • Autobuild: Rename RELEASE_VERSION_NAME -> BUILD_VERSION
    This renames the RELEASE_VERSION_NAME (and all calculation in analyse_git_reference.py beforehand) to BUILD_VERSION instead. The variable does not contain a name (e.g. "Jamulus"), so that's confusing. It isn't solely used for releases either, so BUILD_ seems like a better fit.

  • Autobuild: Rename jamulus_buildversionstring -> JAMULUS_BUILD_VERSION

    • "string" is redundant
    • use uppercase as all other environment variables do
    • using BUILD_VERSION is consistent with the source of the variable (create-release step)
    • prefixing with JAMULUS_ is helpful as other versions are in play in the Workflow as well.
  • Autobuild: Windows: Rename the artifact instead of copying
    This is more consistent with the other platforms and might be slightly faster.

  • Autobuild: Windows: Use Qt version variables instead of hardcoding

  • Autobuild: Linux: Silence apt questions

  • Autobuild: Improve dependency installation log messages

  • Autobuild: Mark local variables as local

  • Autobuild: Workflow: Unify id naming scheme

  • Autobuild: Shorten artifact_deploy_filename variable name

CHANGELOG: Autobuild: Simplified build matrix and cleaned up redundant functionality
(also to be condensed with other autobuild PRs)

Context: Fixes an issue?

Related: #2503

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:
  • 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

hoffie added 14 commits March 23, 2022 11:04
Previously, the build matrix had to provide three different command
lines for the three different build steps. As all command invocations
are normalized across platforms now, we can condense that into a
base_command per matrix entry and will just add the build stage as a
parameter in the generic invocation.
This allows removal of several lines of definitions and makes the
workflow more readable/maintenable.

Related: jamulussoftware#2503
The Workflow used the contains() function in multiple places.
contains() will either perform an in-array check or a substring check
[1]. We are not dealing with arrays, nor do we want a substring check
when checking for 'true'.
Therefore, use an ordinary string comparison here which is more readable
anyways.

[1] https://docs.github.com/en/actions/learn-github-actions/expressions#contains

Related: jamulussoftware#2503
The previous refactorings have adjusted all platform autobuild scripts
to stop using this variable. Therefore, we can now safely drop the
definition from the Workflow.

Related: jamulussoftware#2503
There's no need to set PATH in the prepare step, pass it back to the
workflow, let the workflow pass it back to the build stage and use it
only then.
Therefore, just set PATH right before it's needed in the build step and
drop the Github logic.

Related: jamulussoftware#2503
As no autobuild scripts require setting environment variables across
build steps anymore, we can now safely drop this discouraged
configuration.

In case we ever need to set environment variables across steps again,
there are better ways to do that by now (env files).

Related: jamulussoftware#2503
This renames the RELEASE_VERSION_NAME (and all calculation in
analyse_git_reference.py beforehand) to BUILD_VERSION instead.
The variable does not contain a name (e.g. "Jamulus"), so that's
confusing. It isn't solely used for releases either, so BUILD_ seems
like a better fit.

Related: jamulussoftware#2503
- "string" is redundant
- use uppercase as all other environment variables do
- using BUILD_VERSION is consistent with the source of the variable
  (create-release step)
- prefixing with JAMULUS_ is helpful as other versions are in play in the
  Workflow as well.

Related: jamulussoftware#2503
This is more consistent with the other platforms and might be slightly
faster.

Related: jamulussoftware#2503
@hoffie hoffie added this to the Release 3.9.0 milestone Mar 23, 2022
@hoffie hoffie requested a review from ann0see March 23, 2022 11:55
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Looks good - and CI is green.

@hoffie hoffie merged commit 554a7f3 into jamulussoftware:master Mar 23, 2022
@hoffie hoffie deleted the autobuild-overall-cleanup branch March 23, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants