-
Notifications
You must be signed in to change notification settings - Fork 237
Re add pinning to create-dmg #3511
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
Re add pinning to create-dmg #3511
Conversation
| - name: create-dmg | ||
| changelog_name: create-dmg (macOS) | ||
| get_upstream_version: GH_REPO=create-dmg/create-dmg gh release view --json tagName --jq .tagName | sed -re 's/^v//' | ||
| local_version_regex: (.*CREATEDMG_?VERSION_?TAG\s*=\s*"?)([0-9.]*)("?.*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| local_version_regex: (.*CREATEDMG_?VERSION_?TAG\s*=\s*"?)([0-9.]*)("?.*) | |
| local_version_regex: (.*CREATEDMG_?VERSION_?TAG\s*=\s*"?)(v[0-9.]*)("?.*) |
(not sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe v? then?
Where is it used?
| fi | ||
| brew install "${pkg_version}--"* | ||
| pushd "${dependency_root_folder}" | ||
| sudo make install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the function somewhat not portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this code doesn't need to be portable to a non-Mac. And I assume all Macs will have sudo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. All Macs should have sudo. I just want to make clear that dependencies which don't need make install would need a different function
softins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. I made a branch of my own from it with added logging and ran the autobuild twice. The first time it successfully fetched create-dmg and built it. The second time it picked up the cached create-dmg. I then installed the built artifact and it ran ok.
51d29ab to
157cc00
Compare
| # FIXME: Currently caching is disabled due to an error in the extract step | ||
| brew install create-dmg | ||
| # Install create-dmg | ||
| github_install_dependency "create-dmg/create-dmg" "v${CREATEDMG_VERSION}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to use the version instead of the tag here...
Short description of changes
Moves away from brew to now use native github downloads and installs from create-dmg. Also adds (yet untested) auto updates.
Fixes: #3152
CHANGELOG: SKIP
Context: Fixes an issue?
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?
Test if caching worksIt does. But Qt caching fails - but that's unchanged.Checklist
AUTOBUILD: Please build all targets