Add macOS pkg installer to deployment (#7554) #7555

paulober · 2023-06-08T15:23:54Z

Fixes Add macOS pkg installer #7554

paulober · 2023-06-08T15:30:10Z

See the connected issue for more details on the implementation

paulober · 2023-09-04T10:58:55Z

@vilmibm @williammartin Any updates on this?

williammartin · 2023-09-11T15:54:40Z

@paulober Sorry that your PR is languishing in review purgatory, when I went on parental leave I asked @vilmibm to take ownership but he has since left GitHub. I have not forgotten about it and we are currently figuring out how to deal with our PR backlog with team churn.

paulober · 2024-05-12T15:37:43Z

Will my work now finally after nearly one year be accepted and merged, or should i just delete it and distribute a gh pkg for macOS on my own?

(You are currently maintaining 12 artifacts for linux with 3 different packages per one of the 4 supported architectures.
And macOS people still have to install it manually from a zip folder or rely on a heavy third-party tool.)

paulober · 2024-05-20T15:08:30Z

@andyfeller Hi, if you find some time may you review this PR for merging please?

andyfeller · 2024-05-20T20:35:53Z

@andyfeller Hi, if you find some time may you review this PR for merging please?

@paulober : 🫡 Firstly, let me say thank you for putting the time and effort into this as well as your patience given the impact attrition has taken on the team over the past year.

I'm blocking off time first thing tomorrow morning to review this. Not having a background with .pkg packaging, I also want to compare this to efforts in GitHub Desktop and Mobile areas to compare as a colleague of mine has been working at refactoring how we build and sign within the Mac ecosystem.

andyfeller

@paulober : again thanks for your patience especially as my background in packaging is predominantly OSS with RPMs and DEBs.

few minor nits and some honest questions I'd appreciate your thoughts on while running down hubbers on GitHub Desktop and Mobile to understand these changes in context to those product lines. additionally, I still want to ensure the CLI team has an opportunity to review these.

build/macOS/distribution.xml

andyfeller · 2024-05-21T12:58:05Z

build/macOS/distribution.xml

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="utf-8" standalone="yes"?>
+<installer-gui-script minSpecVersion="1">


For documentation on this file, see Apple Distribution Definition reference

Right, i think we can change the minSpecVersion to 2. But i'll check that first.

Just to be clear, I'm mostly adding reference comments because completely unfamiliar with the spec and I use comments like sticky notes for myself 😅

oh, sorry. didn't know that.

andyfeller · 2024-05-21T13:24:05Z

build/macOS/distribution.xml

+    <title>Github Cli</title>
+    <license file="LICENSE" mime-type="text/plain"/>
+    <options hostArchitectures="arm64,x86_64" customize="never" require-scripts="false" allow-external-scripts="false"/>
+    <domains enable_localSystem="true"/>


Should GitHub CLI be system-wide, per-user, or can be anywhere?

GitHub Desktop zips up the application and on start up will ask the user if they want to move it from Downloads (in my case) to Applications.

Reference

@williammartin : would appreciate your 2 cents here as brew installs packages such that any user on the workstation can add /opt/homebrew to their path where the configuration is per user.

I think I'd be inclined to say /usr/local/bin. When I look at the contents of my /usr/local/bin it feels like a familiar idea. Homebrew seems to have a variety of reasons relating to Apple Silicon to use /opt/homebrew.

I would anticipate the vast majority of users:

Don't feel the need to use amd64 and arm versions of gh

Have a single user account on their personal machine

So I think I would suggest we sidestep that mess and install into /usr/local/bin, knowing that it's on the path already.

What do you think?

build/macOS/distribution.xml

andyfeller · 2024-05-21T13:32:53Z

build/macOS/distribution.xml

+    <pkg-ref id="com.github.cli.pkg" auth="Root" packageIdentifier="com.github.cli">
+        <bundle-version/>
+    </pkg-ref>


What is the impact of this being empty versus being managed from package data?

Reference

Defines the version of the bundles delivered by the parent element. You do not typically specify this element; productbuild inserts it from the actual package data when the product archive is created.

I just checked the code i wrote one year ago and removed some of this xml as it's not needed.
The pkg-ref bundle-version part is generated by productbuild and it also uses this empty bundle-version as the real version can be looked up by installer in the included pkg or the generated named version property on pkg-ref.

script/pkgmacos

andyfeller · 2024-05-21T14:31:30Z

script/pkgmacos

+    cp "LICENSE" "build/macOS/resources"
+    touch .gi
+
+    # build distribution


For information on productbuild, see Apple "Packaging Mac software for distribution"

script/pkgmacos

andyfeller · 2024-05-21T14:36:40Z

script/pkgmacos

+while [ $# -gt 0 ]; do
+  case "$1" in
+  -h | --help )
+    print_help
+    exit 0
+    ;;
+  -* )
+    printf "unrecognized flag: %s\n" "$1" >&2
+    exit 1
+    ;;
+  * )
+    tag_name="$1"
+    shift 1
+    ;;
+  esac
+done


Are there any conditions we should short circuit running this script?

Running it on unsupported OS

If pkgbuild or productbuild aren't installed

...

Not really required but it's better.
I adopted your suggested requirements and as unsupported OS i set all macOS versions prior to macOS 12 (Monterey).
For Linux it's not accounted for as it would exit with code 127 at the start anyways as sw_vers is not available on Linux.

Do you think that's sufficient for this build script or should i also write a safe fail for Linux?

.goreleaser.yml

paulober · 2024-05-21T16:17:02Z

@andyfeller Hi, if you find some time may you review this PR for merging please?

@paulober : 🫡 Firstly, let me say thank you for putting the time and effort into this as well as your patience given the impact attrition has taken on the team over the past year.

I'm blocking off time first thing tomorrow morning to review this. Not having a background with DMG packaging, I also want to compare this to efforts in GitHub Desktop and Mobile areas to compare as a colleague of mine has been working at refactoring how we build and sign within the Mac ecosystem.

Thanks for taking the time to review. I'll address all comments.
Refactoring the macOS target build and signing process sounds like a good project, looking forward to seeing the result.

Co-authored-by: Andy Feller <andyfeller@github.com>

andyfeller · 2024-05-21T16:26:01Z

The only product tangentially relevant to GitHub that built .pkg was Mac OS Git installer.

Co-authored-by: Andy Feller <andyfeller@github.com>

paulober · 2024-05-24T19:51:35Z

Thanks for taking the time to bring the PR of the finish line. Lets see how it works with the next release. I also though about maybe adding a second install script that removes old gh man pages in case some get removed with a new release they wouldn't remain on the users disk if they upgrade with a new pkg...

williammartin · 2024-05-28T16:26:15Z

Unfortunately this broke the release: https://github.com/cli/cli/actions/runs/9272921562/job/25511828490

Looking into it now but if you have any idea that would be great, thanks.

williammartin · 2024-05-28T16:32:06Z

I see in #7555 (comment) the example command and the new makefile target build manpages and completions before building the release and running the packaging script. I would guess that the completions are not available in our macos job and therefore cannot be found. Still investigating.

williammartin · 2024-05-28T16:59:51Z

And I think we don't have completions because they are only build on linux:

cli/.goreleaser.yml

Line 13 in f1dedc9

{{ if ne .Runtime.Goos "linux" }}echo{{ end }} make completions

I think they are only built on linux because they are only packaged up inside the .deb and .rpm files:

cli/.goreleaser.yml

Lines 93 to 98 in f1dedc9

    
           - src: "./share/bash-completion/completions/gh" 
        
             dst: "/usr/share/bash-completion/completions/gh" 
        
           - src: "./share/fish/vendor_completions.d/gh.fish" 
        
             dst: "/usr/share/fish/vendor_completions.d/gh.fish" 
        
           - src: "./share/zsh/site-functions/_gh" 
        
             dst: "/usr/share/zsh/site-functions/_gh"

Perhaps this is as simple as changing that conditional so that they are also produced on macos though it would be a bit of spooky action at a distance that we're expecting to nab the completions at some future point in the release script. Happy to try it for now, leave a comment and revisit.

williammartin · 2024-05-28T17:38:09Z

Created #9136 which I believe fixes this.

paulober · 2024-05-28T18:52:39Z

Oh right, i guess that's why i originally added: #7555 (comment)

paulober · 2024-05-28T18:52:54Z

Sorry, shouldn't have removed it.

williammartin · 2024-05-28T18:55:36Z

No worries, Workflows mostly being collections of cobbled together scripts makes things extremely challenging to write and review. I'm not at all surprised we missed something.

williammartin · 2024-05-29T10:09:18Z

It looks like we built the pkg successfully but we failed to sign it: https://github.com/cli/cli/actions/runs/9284093254/job/25545720127#step:9:11

Looking at the workflow we expect something called APPLE_DEVELOPER_INSTALLER_ID to be set:

cli/.github/workflows/deployment.yml

Line 122 in 3620e79

APPLE_DEVELOPER_INSTALLER_ID: ${{ vars.APPLE_DEVELOPER_INSTALLER_ID }}

but we don't have any variable with that name.

Is it supposed to be vars.APPLE_DEVELOPER_ID?

paulober · 2024-05-29T10:12:21Z

No, because to sign installers you need a special purpose certificate from Apple. In the portal where you also created your APPLE_DEVELOPER_ID certificate you can create a new one for installer signing. It should start with Developer ID Installer ...

williammartin · 2024-05-29T10:17:34Z

Thanks. Because Apple enforce a limit of 5 certificates, the CLI team does not own or manage them. If this is a different certificate as you say, I don't think it's likely this will be resolved today. We may have to either revert this or release without it being signed in the interim. Will look into this now but I suspect the people with the answer are in the US timezone.

paulober · 2024-05-29T10:23:13Z

Ok understand. Not sure what you mean with a limit of 5 certificate. I have about 15 active. Alternatively i could offer to sign the first release manually with my Developer ID Installer certificate or we release it unsigned (maybe with a little note in the release).

williammartin · 2024-05-29T10:29:08Z

Well, the limit is documented here and that is what has been communicated to our team.

I think it would be better to release unsigned than signed by your personal cert, thanks. Unsigned may be acceptable as an MVP that we improve. The binaries themselves weren't signed from the beginning either. I'll discuss with @andyfeller when he comes online.

paulober · 2024-05-29T10:31:05Z

Well, the limit is documented here and that is what has been communicated to our team.

Oh right, didn't know that.

williammartin · 2024-05-29T10:34:13Z

And as you can see from https://github.blog/2023-01-30-action-needed-for-github-desktop-and-atom-users/, we really, really want to take care of these limited certs 😬

williammartin · 2024-05-29T12:14:02Z

@paulober when you sign your binaries, how do you ensure that the certificate is in the keychain and accessible. For example, my understanding of our release process is that we import the certificate used for signing and then later when we codesign, codesign checks the keychain and uses that cert for signing.

I believe the same is true for productbuild based on https://thegreyblog.blogspot.com/2014/06/os-x-creating-packages-from-command_2.html "Signing the Product Archive" section. Perhaps the certificate is the same, but then I would also expect we need to allow productbuild access to it on both these lines:

cli/.github/workflows/deployment.yml

Lines 91 to 92 in 3620e79

    
                     security import "$RUNNER_TEMP/cert.p12" -k "$keychain" -P "$APPLE_APPLICATION_CERT_PASSWORD" -T /usr/bin/codesign 
        
                     security set-key-partition-list -S "apple-tool:,apple:,codesign:" -s -k "$keychain_password" "$keychain"

paulober · 2024-05-29T12:38:45Z

If I sign from terminal manually i normally install the certificate via drag and drop. (not applicable here)
Yes, productbuild and codesign both pull the certificate from the security keychain .

It should not be the same as the Developer ID Installer and Developer ID Application are different certificates with different "permissions" (though i never tried signing a binary with a Developer ID Installer certificate).
And yes for signing without sudo, -T /usr/bin/productbuild needs to be added to the security import command.
Concerning the second command I think we don't have to change stuff there and i'm not sure if codesign: is even a valid value here

williammartin · 2024-05-29T13:02:05Z

Thanks. I think we're in shared understanding. I also believe we need another cert and we need to load it into the keychain.

Concerning the second command I think we don't have to change stuff there and i'm not sure if codesign: is even a valid value here

Yeh I'm not sure about that either. The docs for set-key-partition-list say:

The "partition list" is an extra parameter in the ACL which limits access to the key based on an application's code signature. You must present the keychain's password to change a partition list. If you'd like to run /usr/bin/codesign with the key, "apple:" must be an element of the partition list.

Which doesn't indicate the need for codesign, nor can I find mention of people doing that in other places: https://stackoverflow.com/questions/39868578/security-codesign-in-sierra-keychain-ignores-access-control-settings-and-ui-p

andyfeller · 2024-05-29T13:22:12Z

Thanks. I think we're in shared understanding. I also believe we need another cert and we need to load it into the keychain.

Agreed 😞

Relevant docs: Apple "Packaging Mac Software for distribution > Build an Installer package":

If you choose to distribute your product in an Installer package, start by determining your Installer signing identity. Choose the right identity for your distribution channel:

If you’re distributing an app on the Mac App Store, use a Mac Installer Distribution signing identity. This is named 3rd Party Mac Developer Installer: , where identifies your team.

If you’re distributing a product independently, use a Developer ID Installer-signing identity. This is named Developer ID Installer: , where identifies your team.

Additional doc on Creating Developer ID certificates

williammartin · 2024-05-29T14:20:28Z

The "partition list" is an extra parameter in the ACL which limits access to the key based on an application's code signature. You must present the keychain's password to change a partition list. If you'd like to run /usr/bin/codesign with the key, "apple:" must be an element of the partition list.

Some answers in here indicate that codesign should be part of the partition list:+ https://stackoverflow.com/a/40870033

williammartin · 2024-05-29T15:42:22Z

@paulober when you say:

Ok understand. Not sure what you mean with a limit of 5 certificate. I have about 15 active

When getting certificates now we noticed on the Apple developer website and the docs there was a difference between Mac Installer Distribution signing identity and Developer ID Installer-signing identity. The former can be used for distributing on the App Store and it didn't seem to have limitations and the latter required us to be the account owner and seemed much more restricted. Perhaps that is the difference in our use cases?

paulober · 2024-05-29T16:30:46Z

Some answers in here indicate that codesign should be part of the partition list:+ https://stackoverflow.com/a/40870033

Strange, i'm sill looking for a documentation about where to find these partitions and which are required for which usecase/tools.

paulober · 2024-05-29T20:01:54Z

I'm not sure if i understand you question correctly. But I guess you where asking about the differences between these two certificates in our case:
So the Developer ID Installer named certificates have a different purpose in their metadata (1.2.840.113635.100.4.13) whereas the Developer ID Application named one have Code Signing ( 1.3.6.1.5.5.7.3.3 ) as their purpose. So if you try to sign a pkg with productsign --sign "Developer ID Application ..." you'll get an error: An installer signing identity (not an application signing identity) is required for signing flat-style products. And i guess Gatekeeper also don't like it.

So you would need to create a csr and upload it to apple dev center and request a Developer ID Installer which you can then inject like the code-signing certificate into the action.

williammartin · 2024-05-30T08:59:55Z

I'm not sure if i understand you question correctly. But I guess you where asking about the differences between these two certificates in our case

Ah not quite. I was referring to the fact there are two different identities that can be used to sign .pkgs. One is Mac Installer Distribution signing identity which has no limit to the number you can create, and seems to be used to sign pkgs for the Mac store, and the other is Developer ID Installer-signing identity which seems to be limited to 5. I just wondered if maybe you had 15 of the first one, whereas we needed the second since we are distributing outside the mac store.

From the docs:

If you choose to distribute your product in an Installer package, start by determining your Installer signing identity. Choose the right identity for your distribution channel:

If you’re distributing an app on the Mac App Store, use a Mac Installer Distribution signing identity. This is named 3rd Party Mac Developer Installer: , where identifies your team.

If you’re distributing a product independently, use a Developer ID Installer-signing identity. This is named Developer ID Installer: , where identifies your team.

These are both separate from Developer ID Application identity which we currently use to sign the binaries, and we are in agreement that they cannot be used to sign pkgs.

paulober · 2024-05-31T08:02:56Z

Oh, yes, I might overlooked that one. So for my understanding the Installer Distribution one is not that restricted because Apple can take a look at the software before it is distributed to the end user. Whereas a Developer ID Installer certificate gives you much higher responsibility.

And by "about 15 certificates" I meant the amount of all certificates across all types as i though you meant this limit.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://togithub.com/cli/cli) | minor | `v2.49.2` -> `v2.50.0` | --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.50.0`](https://togithub.com/cli/cli/releases/tag/v2.50.0): GitHub CLI 2.50.0 [Compare Source](https://togithub.com/cli/cli/compare/v2.49.2...v2.50.0) #### What's Changed - Refactor git credential flow code by [@williammartin](https://togithub.com/williammartin) in [cli/cli#9089 - feat: add json output for `gh pr checks` by [@nobe4](https://togithub.com/nobe4) in [cli/cli#9079 - Rework first auth tests with new gitcredential abstractions by [@williammartin](https://togithub.com/williammartin) in [cli/cli#9095 - list the various alias permutations for the command and subcommands, via '--help' and 'gh reference' by [@gabemontero](https://togithub.com/gabemontero) in [cli/cli#8824 - Removed tty message when checking for extension upgrades by [@leevic31](https://togithub.com/leevic31) in [cli/cli#9088 - Fix doc bug for gh run watch by [@jasonodonnell](https://togithub.com/jasonodonnell) in [cli/cli#9052 - feat: add support for stateReason in `gh pr view` by [@nobe4](https://togithub.com/nobe4) in [cli/cli#9080 - fix: rename the `Attempts` field to `Attempt`; expose in `gh run view` and `gh run ls` by [@cawfeecake](https://togithub.com/cawfeecake) in [cli/cli#8905 - Update regex in changedFilesNames to handle quoted paths by [@anda3](https://togithub.com/anda3) in [cli/cli#9115 - Add a `gh variable get FOO` command by [@arnested](https://togithub.com/arnested) in [cli/cli#9106 - Add macOS pkg installer to deployment ([#7554](https://togithub.com/cli/cli/issues/7554)) by [@paulober](https://togithub.com/paulober) in [cli/cli#7555 - Add integration tests for `gh attestation verify` shared workflow use case by [@malancas](https://togithub.com/malancas) in [cli/cli#9107 - Add build provenance for gh CLI releases by [@malancas](https://togithub.com/malancas) in [cli/cli#9087 - build(deps): bump github.com/gabriel-vasile/mimetype from 1.4.3 to 1.4.4 by [@dependabot](https://togithub.com/dependabot) in [cli/cli#9124 - Build completions during release on macos by [@williammartin](https://togithub.com/williammartin) in [cli/cli#9136 - Clarify Mac OS Installer packages are unsigned by [@andyfeller](https://togithub.com/andyfeller) in [cli/cli#9140 #### New Contributors - [@gabemontero](https://togithub.com/gabemontero) made their first contribution in [cli/cli#8824 - [@jasonodonnell](https://togithub.com/jasonodonnell) made their first contribution in [cli/cli#9052 - [@anda3](https://togithub.com/anda3) made their first contribution in [cli/cli#9115 - [@arnested](https://togithub.com/arnested) made their first contribution in [cli/cli#9106 - [@paulober](https://togithub.com/paulober) made their first contribution in [cli/cli#7555 **Full Changelog**: cli/cli@v2.49.2...v2.50.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/izumin5210/dotfiles).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: izumin5210-update-aqua-checksum[bot] <169593670+izumin5210-update-aqua-checksum[bot]@users.noreply.github.com>

paulober requested a review from a team as a code owner June 8, 2023 15:23

paulober requested review from williammartin and removed request for a team June 8, 2023 15:23

cliAutomation added the external pull request originating outside of the CLI core team label Jun 8, 2023

cliAutomation added this to Needs review 🤔 in The GitHub CLI Jun 8, 2023

paulober force-pushed the feature-macos-pkg-installer branch from 3abfe1a to a2e6927 Compare June 8, 2023 15:27

williammartin added blocked discuss Feature changes that require discussion primarily among the GitHub CLI team labels Jun 8, 2023

williammartin requested review from vilmibm and removed request for williammartin June 29, 2023 19:23

Add macOS pkg installer to deployment

f1c3534

paulober force-pushed the feature-macos-pkg-installer branch from a2e6927 to f1c3534 Compare July 7, 2023 10:01

paulober added 2 commits September 4, 2023 12:59

Merge branch 'trunk' into feature-macos-pkg-installer

53c83fd

Fix typo in macOS pkg installer build script

775476d

williammartin requested review from williammartin and removed request for vilmibm September 11, 2023 15:57

samcoe removed the discuss Feature changes that require discussion primarily among the GitHub CLI team label Oct 30, 2023

cli deleted a comment Nov 1, 2023

Merge branch 'trunk' into feature-macos-pkg-installer

630ab13

andyfeller reviewed May 21, 2024

View reviewed changes

Update pkg title

b1a6ce3

Co-authored-by: Andy Feller <andyfeller@github.com>

Update choice title

aeb0ac7

Co-authored-by: Andy Feller <andyfeller@github.com>

andyfeller merged commit 1bc3cfa into cli:trunk May 24, 2024
8 checks passed

paulober deleted the feature-macos-pkg-installer branch May 24, 2024 21:24

williammartin removed the blocked label May 28, 2024

williammartin mentioned this pull request May 28, 2024

Build completions during release on macos #9136

Merged

		@@ -0,0 +1,19 @@
		<?xml version="1.0" encoding="utf-8" standalone="yes"?>
		<installer-gui-script minSpecVersion="1">

Add macOS pkg installer to deployment (#7554) #7555

Add macOS pkg installer to deployment (#7554) #7555

Conversation

paulober commented Jun 8, 2023

paulober commented Jun 8, 2023

paulober commented Sep 4, 2023

williammartin commented Sep 11, 2023

paulober commented May 12, 2024

paulober commented May 20, 2024

andyfeller commented May 20, 2024 • edited

andyfeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulober May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulober May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulober commented May 21, 2024

andyfeller commented May 21, 2024

paulober commented May 24, 2024

williammartin commented May 28, 2024

williammartin commented May 28, 2024

williammartin commented May 28, 2024 • edited

williammartin commented May 28, 2024

paulober commented May 28, 2024

paulober commented May 28, 2024

williammartin commented May 28, 2024

williammartin commented May 29, 2024

paulober commented May 29, 2024

williammartin commented May 29, 2024

paulober commented May 29, 2024

williammartin commented May 29, 2024

paulober commented May 29, 2024

williammartin commented May 29, 2024

williammartin commented May 29, 2024

paulober commented May 29, 2024

williammartin commented May 29, 2024

andyfeller commented May 29, 2024

williammartin commented May 29, 2024

williammartin commented May 29, 2024

paulober commented May 29, 2024

paulober commented May 29, 2024

williammartin commented May 30, 2024

paulober commented May 31, 2024

andyfeller commented May 20, 2024 •

edited

paulober May 21, 2024 •

edited

paulober May 21, 2024 •

edited

williammartin commented May 28, 2024 •

edited