Build: Cache brew/create-dmg install #2420

hoffie · 2022-02-21T00:03:18Z

Short description of changes

The Mac build installs create-dmg via brew. The install is already version-pinned.
This PR caches the resulting install and adds the file which does the version pinning to the cache key.

Context: Fixes an issue?

Fixes #2412

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

No
CHANGELOG: SKIP
(I've included this PR in #2207 (comment))

Status of this Pull Request

Ready.

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 · 2022-02-21T01:00:27Z

Doesn't work yet, probably some path missing:
https://github.com/jamulussoftware/jamulus/runs/5267841353?check_suite_focus=true#step:9:1294

pljones · 2022-02-21T20:31:46Z

Mac installs mean nothing to me. Was there something specific you were thinking of?

hoffie · 2022-02-21T20:43:48Z

Mac installs mean nothing to me. Was there something specific you were thinking of?

Hm, if I only knew... I might have confused this PR with another one. Sorry. Will ping softins once all problems with this PR are fixed.

ann0see · 2022-02-23T18:52:31Z

mac/deploy_mac.sh

+        brew extract --version="${create_dmg_version}" create-dmg homebrew/cask
+        brew install --build-bottle --formula "${formula}"
+        brew bottle "${formula}"


Isn't it duplicate work if we run reinstall?

Why can't we just use an if/else?

The problem is that --build-bottle seems to disable postinst scripts. I'm not sure if aqt uses those at all, but it might at some point, so I'd rather be on the safe side here.
There's also the option of running the postinst scripts explicitly. I chose the reinstall path though as it most closely ensures that both paths are really similar.

I should probably add a comment though.

The plain (re)install is really small, btw. Cloning the repo and searching for the right version seems to be the resource-intensive part.

Side note: The PR still doesn't work as it should, therefore it's still a draft again. When testing with a fake build (no compile), the logic works, while in the real build it doesn't (= always a fresh build). I'm probably missing something.

Changed that part now to make it clearer (uninstall package & add comment + regular install later).
Testing still not finalized, will continue tomorrow.

mac/deploy_mac.sh

hoffie · 2022-02-27T11:28:33Z

Finally.

Please do not squash-merge.

First run:

Cache miss: https://github.com/jamulussoftware/jamulus/runs/5349900281?check_suite_focus=true#step:4:10
Fresh create-dmg download/build: https://github.com/jamulussoftware/jamulus/runs/5349900281?check_suite_focus=true#step:9:1627

Second run:

Cache hit: https://github.com/jamulussoftware/jamulus/runs/5349974155?check_suite_focus=true#step:4:15
Use of existing create-dmg package: https://github.com/jamulussoftware/jamulus/runs/5349974155?check_suite_focus=true#step:9:1628

pljones

I can't find the Action in hoffie:actions-cache-create-dmg for this to check but it seems clean in the PR build.

ann0see

To me it still seems a bit strange to install and then uninstall the thing. But I'll probably need to be happy with that. Another possible step would be to ask on the brew forum how they do caching.

hoffie · 2022-02-27T21:15:50Z

I can't find the Action in hoffie:actions-cache-create-dmg for this to check but it seems clean in the PR build.

The links I provided pointed to two different runs as part of this PR.
I don't think the branch as its own runs in my repo as the branch name doesn't start with autobuild*.

To me it still seems a bit strange to install and then uninstall the thing.

If you felt better with the other path (if (no package) { download; build; install; manual postinstall } else { install }) I can adjust that. The downside would be that the gap between cache-hit and cache-miss runs would be larger. Should I change it?

Another possible step would be to ask on the brew forum how they do caching.

The cache part isn't that complex at all. What makes most of the lines is the version-specific download, build and packaging.

ann0see · 2022-02-27T21:21:31Z

Hmm. If your approach is better performance wise, we should go with that (and make the comment even clearer why we didn't choose the other approach).

ann0see · 2022-02-27T21:23:50Z

mac/deploy_mac.sh

+        # uninstall again to ensure that we can install the package freshly without leftovers afterwards:
+        brew uninstall "${pkg_version}"


Something like "since brew install doesn't run postinst scripts, we are uninstalling it before later on installing it again [...]"

I've reworked the comment again.

(It's not about performance, it's more about correctness)

Fixes jamulussoftware#2412

Failing to do so will lead to a missing cache reset when updating e.g. create-dmg's vesion. Related: jamulussoftware#2412

softins

I looked at the workflow log and could see that on the first run, no cached files were found.

I then selected "Re-run all jobs" and observed the Mac build. This time it found the cached files, and the build continued successfully.

hoffie added this to the Release 3.9.0 milestone Feb 21, 2022

hoffie requested review from ann0see and pljones February 21, 2022 00:03

hoffie force-pushed the actions-cache-create-dmg branch 2 times, most recently from 0223724 to 2fa0a6c Compare February 21, 2022 00:38

hoffie marked this pull request as draft February 21, 2022 01:00

hoffie force-pushed the actions-cache-create-dmg branch 5 times, most recently from 29c78fb to 1167911 Compare February 21, 2022 11:37

This comment was marked as outdated.

Sign in to view

hoffie force-pushed the actions-cache-create-dmg branch 11 times, most recently from 3420e35 to 276fb61 Compare February 21, 2022 19:48

hoffie force-pushed the actions-cache-create-dmg branch from 276fb61 to 8afb73c Compare February 21, 2022 20:38

hoffie force-pushed the actions-cache-create-dmg branch 3 times, most recently from df9c00a to 10b6e8f Compare February 21, 2022 22:12

ann0see reviewed Feb 23, 2022

View reviewed changes

hoffie force-pushed the actions-cache-create-dmg branch 2 times, most recently from 9cf292f to 10c6cdc Compare February 27, 2022 01:15

hoffie marked this pull request as ready for review February 27, 2022 11:28

hoffie requested a review from ann0see February 27, 2022 11:28

pljones reviewed Feb 27, 2022

View reviewed changes

ann0see reviewed Feb 27, 2022

View reviewed changes

hoffie added 2 commits February 28, 2022 16:11

Build: Cache brew/create-dmg install

be7cdf7

Fixes jamulussoftware#2412

Build: Make mac/deploy_mac.sh part of the actions/cache key

6b48f6d

Failing to do so will lead to a missing cache reset when updating e.g. create-dmg's vesion. Related: jamulussoftware#2412

hoffie force-pushed the actions-cache-create-dmg branch from 8cd1b29 to 6b48f6d Compare February 28, 2022 15:12

softins approved these changes Feb 28, 2022

View reviewed changes

ann0see approved these changes Feb 28, 2022

View reviewed changes

hoffie mentioned this pull request Feb 28, 2022

Switch to create-dmg #2207

Merged

5 tasks

hoffie merged commit 259545f into jamulussoftware:master Feb 28, 2022

hoffie deleted the actions-cache-create-dmg branch February 28, 2022 21:30

		# uninstall again to ensure that we can install the package freshly without leftovers afterwards:
		brew uninstall "${pkg_version}"

Build: Cache brew/create-dmg install #2420

Build: Cache brew/create-dmg install #2420

Uh oh!

Conversation

hoffie commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

hoffie commented Feb 21, 2022

Uh oh!

This comment was marked as outdated.

pljones commented Feb 21, 2022

Uh oh!

hoffie commented Feb 21, 2022

Uh oh!

ann0see Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

hoffie Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoffie Feb 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoffie commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones left a comment

Choose a reason for hiding this comment

Uh oh!

ann0see left a comment

Choose a reason for hiding this comment

Uh oh!

hoffie commented Feb 27, 2022

Uh oh!

ann0see commented Feb 27, 2022

Uh oh!

ann0see Feb 27, 2022

Choose a reason for hiding this comment

Uh oh!

hoffie Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hoffie commented Feb 21, 2022 •

edited

Loading

hoffie Feb 23, 2022 •

edited

Loading

hoffie commented Feb 27, 2022 •

edited

Loading