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: Combine and simplify Windows build scripts #2502

Merged

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Mar 13, 2022

Short description of changes

  • Move all autobuild/windows/* scripts to a single autobuild/windows.ps1 script which is called for the different stages (setup/build/get-artifacts).
  • Condense redundant parameter parsing into a single step
  • Simplify jack build naming
  • Create functions with proper names for larger steps
  • Inline Github artifact output definition as it's shorter that way
  • Make coding style more consistent (spaces, curly braces)

CHANGELOG: Autobuild: Refactored and simplified Windows build scripts.
(will be condensed...)

Notes:

  • This is a rather large Diff which cannot be easily followed. I'm sorry for that, but I don't see any way to make this better. I'm usually a fan of having individual commits for sub-changes, but in this case it would have required artificially creating "bad" intermediate states just to fix them in a later commit. Therefore, the diff is rather large, but I'm hoping that the resulting file is way more readable than what we had before.
  • I've kept the script in autobuild/ as it used to be. We can also think of moving all autobuild scripts (now that they are only a single script) to their platform folder, e.g. move autobuild/windows.ps1 to windows/autobuild.ps1. If wanted, I can do this as part of this PR.
  • This PR drops some error checking code which is not relevant for Github-initiated builds as it makes the scripts way shorter and more readable. They error checks sound like they may have been relevant during development or when running locally. However, the scripts were not designed to run locally as they make use of the Github-setup build environments and techniques (e.g. magic set-output lines; it's not impossible, but it wasn't straight-forward either; it's still possible and might be a bit more straight-forward at least).
  • Doing this for Windows is obviously just the first step. After this PR, the other platforms will be a bit unaligned in how the scripts are run. I still chose to submit this now because I consider it ready and I think it's easier to review and reason about. PRs for the other platforms will be done in the coming days (even if I was unable to manage to complete them timely for whatever reasons, all platforms are in working state).
  • Once all platforms are converted to the new model, I'll simplify the build matrix (it will be enough to supply a single base command: entry for each platform). I will also drop a number of unneeded commands (GITHUB_WORKSPACE extra logic).

Context: Fixes an issue?

#2282 (reply in thread)
Related: #2503

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

No.

Status of this Pull Request

What is missing until this pull request can be merged?

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 hoffie added this to the Release 3.9.0 milestone Mar 13, 2022
@hoffie hoffie requested a review from ann0see March 13, 2022 11:21
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.

I'd still be more happy if we let it in a windows specific folder for now. Maybe in future, we need to add other files like documentation?

autobuild/windows.ps1 Outdated Show resolved Hide resolved
@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch 2 times, most recently from 980caee to 0598dae Compare March 13, 2022 12:24
@ann0see
Copy link
Member

ann0see commented Mar 13, 2022

I can't find the JACK build artifact of this run. Maybe there's a naming clash?

@hoffie hoffie marked this pull request as draft March 13, 2022 18:31
@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch from 0598dae to 97f9d59 Compare March 13, 2022 19:38
@hoffie
Copy link
Member Author

hoffie commented Mar 13, 2022

I can't find the JACK build artifact of this run. Maybe there's a naming clash?

Thanks for the pointer. Build step three lacked the necessary -BuildOption. Added that now and waiting for CI.

@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch from 97f9d59 to a1d2b51 Compare March 13, 2022 19:56
@hoffie hoffie marked this pull request as ready for review March 13, 2022 20:48
@hoffie
Copy link
Member Author

hoffie commented Mar 13, 2022

Build step three lacked the necessary -BuildOption. Added that now and waiting for CI.

That fixed it.
I've also compared the resulting binary sizes with the most recent master CI run and their Jamulus.exe sizes match exactly (ASIO: 2532352, JACK: 2514944). See PR description for links.
I'm fairly confident that this PR is good to go now.

In addition, I've checked the resulting Jamulus.exe files for JACK references (and lack thereof) -- all good.

@ann0see
Copy link
Member

ann0see commented Mar 13, 2022

Great! It works now.

Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>
@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch from a1d2b51 to cdd2a4b Compare March 13, 2022 21:58
.github/autobuild/windows.ps1 Show resolved Hide resolved
.github/autobuild/windows.ps1 Outdated Show resolved Hide resolved
Comment on lines +138 to +139
echo "Copying artifact to ${artifact_deploy_filename}"
cp ".\deploy\Jamulus*installer-win.exe" ".\deploy\${artifact_deploy_filename}"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the deploy script could be adapted to create the correct name (but that's for another PR)

.github/autobuild/windows.ps1 Outdated Show resolved Hide resolved
@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch from cdd2a4b to f3ef254 Compare March 13, 2022 22:34
Related: jamulussoftware#2503

- Move all autobuild/windows/* scripts to a single .github/autobuild/windows.ps1
  script which is called for the different stages
  (setup/build/get-artifacts).

- Condense redundant parameter parsing into a single step

- Simplify jack build naming

- Create functions with proper names for larger steps

- Inline Github artifact output definition as it's shorter that way

- Make coding style more consistent (spaces, curly braces)
@hoffie hoffie force-pushed the autobuild-merge-windows-scripts branch from f3ef254 to da1b741 Compare March 13, 2022 22:50
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.

Yes, I think that ok.

@ann0see ann0see merged commit 1a3a72a into jamulussoftware:master Mar 14, 2022
@hoffie hoffie deleted the autobuild-merge-windows-scripts branch March 19, 2022 20:22
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