Add MacOS notarization support #3725

mbacchi · 2019-10-16T19:42:20Z

Fixes issue brave/brave-browser#5177
Related brave/brave-browser#5485

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

build/mac/notarize_dmg_pkg.py

bridiver · 2019-10-23T18:54:01Z

patches/chrome-installer-mac-BUILD.gn.patch

+   ]
+ }
+
+-_packaging_dir = "$root_out_dir/$chrome_product_full_name Packaging"


what is the purpose of this change?

In order to change the path of the packaging directory to have underscores rather than spaces in the directory name.

I understand what it's doing, but why do we need to do that?

bridiver · 2019-10-23T18:54:25Z

patches/chrome-installer-mac-signing-config.py.in.patch

+     @property
+     def packaging_dir(self):
+         """Returns the path to the packaging and installer tools."""
+-        return '{.product} Packaging'.format(self)


same as above, why are we changing this path?

bridiver · 2019-10-23T18:56:01Z

patches/chrome-installer-mac-signing-pipeline.py.patch

+         '--volname', config.app_product,
+         '--icon', os.path.join(packaging_dir, icon_file),
+         '--copy', '{}:/'.format(app_path),
+-        '--copy',


why do we need to remove this?

The keystone_install.sh part of the command isn't copied or called, leaving it in caused a build break during my testing.

where did it break? Can we remove it later or use a dummy script just to satisfy the copy? We need to find other ways to avoid patches like this

OK so we can do a touch (to create blank files for keystone_install and chrome_dmg_background.png) and then remove it before packaging

bridiver · 2019-10-23T18:56:40Z

patches/chrome-installer-mac-signing-pipeline.py.patch

+                     notarize.staple(package_path)
+        else:
+            # Copy the notarized app to the orig_paths.output dir where the user expects it.
+            commands.copy_files(dest_dir, orig_paths.output)


we can copy files in a separate target after the signing

In the brave/build/mac/BUILD.gn file, I output signed .dmg/.pkg files into the signed subdir, then I notarize them from that subdir, and add a directory that outputs the notarized .dmg/.pkg files. This also allows us to avoid a duplicate outputs error in GN because if notarization is not being done, we want the same file names to be created in the src/out/Release output directory whether only signing is requested or also if notarization is requested. To work around that I use the copy_dmg_pkg target to determine which files to copy to the output directory, depending on whether notarize is defined.

Can't you just use a different directory to avoid the duplicate outputs? Or better yet, why don't we just plug our own notarize.py into the python path and use the existing calls with do_notarization enabled?
If we did need to keep the patch for some reason we never add comments in patches and there's no reason for the else because in this case we know that do_notarization is false. When we do have to patch files the patches should be as small as possible.

and actually it's not clear to me how this solves the problem anyway because now this target needs to specify the outputs that the copy would have

@bridiver can we keep this as-is for now? I believe because we use --disable-packaging (to set custom icons, path to install location (/Applications/ etc)). We could follow up on this later (I can create an issue and @mbacchi can make sure it's tracked in the regular Devops meeting). Since this works as-is, this would eat some time ☹️

it actually doesn't work as-is. It breaks gn's ability to known when things need to be updated because this script no longer specifies the correct outputs

the other target that has the same output won't necessarily run when it should now

bridiver · 2019-10-23T18:57:23Z

patches/chrome-installer-mac-signing-model.py.patch

+         Returns:
+             Path to the packaging directory.
+         """
+-        return os.path.join(self.input, '{} Packaging'.format(config.product))


same as above, why are we adding underscores everywhere?

I couldn't make GN call the build/mac/notarize_dmg_pkg.py script (as an action) with an argument that contained spaces (i.e. Brave Browser Nightly Packaging). It needs the packaging directory as the argument --pkgdir which failed with spaces.

there shouldn't be any problem with spaces, you just need to quote it properly. Can you link me to the gn target where you are using it?

It is used specifically as an argument to the notarize_dmg_pkg.py script here: https://github.com/brave/brave-core/pull/3725/files#diff-39842cb5e3be671a7dd386e445aca164R328

bridiver · 2019-10-23T19:03:27Z

build/mac/notarize_dmg_pkg.py

@@ -0,0 +1,153 @@
+#!/usr/bin/env python


was this copied from chrome? It looks like it was and I'm curious why, but also we can't copy a chromium file and use our own copyright. That's a violation of their copyright

No, it merges the brave/script/signing_helper.py create_config section with the notarization calls from chrome/installer/mac/signing/pipeline.py, but otherwise it is not copied from Chromium.

if it includes any copied chromium code we should include their copyright notice as well

@bridiver only a few lines are similar to the Chromium one, the rest is all ours- it's def not copied. Looks good to me 👍

We could reference the area which has similarities and say something like "Logic based on Chromium one at src/components/blah/installer/" to attribute it

this file is also in the wrong place. Scripts should go in the scripts directory

bridiver · 2019-10-23T19:04:34Z

build/mac/BUILD.gn

+    ]
+  }
+
+  copy("copy_notarize_script") {


why are we copying the script?

In order to run it from the packaging directory where the chrome/installer/mac/signing Python modules have been copied. If we ran it out of the brave/build/mac directory those Python modules aren't accessible to be imported.

copying the file doesn't change where it runs from and it shouldn't be in build/mac in the first place. The scripts go in brave/scripts and they work just fine there

I guess chrome also does this for some reason so don't worry about it for now

bsclifton · 2019-10-29T19:24:09Z

build/mac/BUILD.gn

+      provisioning_profile = "//brave/build/mac/dummy.provisionprofile"
+    }
+
+    script = "$packaging_dir/notarize_dmg_pkg.py"


(per Zoom review) Can we do the rebase_path on this too?

well crap. @bridiver given this, I'd like to propose we keep the underscores in right now- but create a follow up issue and prioritize it on the Devops board

I tried a few invocations of this:
rebase_path("$packaging_dir/notarize_dmg_pkg.py") a well as rebase_path("notarize_dmg_pkg.py", "$packaging_dir") but in both cases I get the error:

python ../../brave/build/mac/notarize_dmg_pkg.py ... Traceback (most recent call last): File "../../brave/build/mac/notarize_dmg_pkg.py", line 27, in <module> from signing import config, commands, model, notarize, pipeline, signing # noqa: E402 ImportError: No module named signing

This shows that its running the python script from the root_build_dir rather than the $packaging_dir. I think the first argument to the python interpreter in an action() target is the script but having spaces in the python script path may not be supported. I haven't seen any examples of this in the Brave or Chromium codebase after a little searching.

bsclifton

I think this is a good v1 attempt; there are two things called out which we could do in follow ups:

Reduce patch size in patches/chrome-installer-mac-signing-pipeline.py.patch; we can do a touch and let the script copy an empty background/keystone script and then remove at a later point
Get rid of the underscores for the action (if possible). This might mean reworking things to try use more of what Chrome has in place already (which could be tough; since we do --disable-packaging)

Fixes: brave/brave-browser#5177

* extend and use GetBraveSigningConfig from signing_helper.py * remove commented out section from create_config * remove check for do_notarization

mbacchi requested a review from bridiver as a code owner October 16, 2019 19:42

mbacchi requested review from simonhong, mkarolin, bsclifton, jhreis and mihaiplesa October 16, 2019 19:42

mbacchi self-assigned this Oct 16, 2019

mkarolin reviewed Oct 18, 2019

View reviewed changes

build/mac/notarize_dmg_pkg.py Outdated Show resolved Hide resolved

mkarolin reviewed Oct 18, 2019

View reviewed changes

build/mac/notarize_dmg_pkg.py Outdated Show resolved Hide resolved

mkarolin reviewed Oct 18, 2019

View reviewed changes

build/mac/notarize_dmg_pkg.py Outdated Show resolved Hide resolved

bsclifton force-pushed the issue_5177 branch from 3354510 to aae31dd Compare October 18, 2019 23:40

mbacchi mentioned this pull request Oct 21, 2019

Upgrade Sparkle to support hardened runtime signing, sign additional Sparkle binaries brave/brave-browser#6572

Closed

mbacchi force-pushed the issue_5177 branch from 0457005 to f2b2a7c Compare October 22, 2019 17:26

bridiver reviewed Oct 23, 2019

View reviewed changes

mbacchi mentioned this pull request Oct 29, 2019

Add MacOS notarization brave/brave-browser#5485

Merged

28 tasks

bsclifton reviewed Oct 29, 2019

View reviewed changes

bsclifton approved these changes Oct 29, 2019

View reviewed changes

bsclifton added this to the 0.73.x - Nightly milestone Oct 30, 2019

mbacchi added 4 commits October 30, 2019 16:48

Add MacOS notarization

7437d40

Fixes: brave/brave-browser#5177

Update patch for Chromium 78

9e9b0e1

Correct patch for Chromium 78; finalize non-notarization path

b35d6da

Update script to adhere to PEP8 guidelines

86147dc

mihaiplesa force-pushed the issue_5177 branch from f2b2a7c to b228ea5 Compare October 30, 2019 16:48

Review updates

f7a6516

* extend and use GetBraveSigningConfig from signing_helper.py * remove commented out section from create_config * remove check for do_notarization

mihaiplesa force-pushed the issue_5177 branch from b3c46db to f7a6516 Compare October 30, 2019 17:39

mihaiplesa approved these changes Oct 30, 2019

View reviewed changes

mihaiplesa merged commit 7f7a2a8 into master Oct 30, 2019

mihaiplesa deleted the issue_5177 branch October 30, 2019 17:45

brave-builds pushed a commit that referenced this pull request Nov 1, 2019

Uplift of #3725 (squashed) to dev

e86d153

brave-builds mentioned this pull request Nov 1, 2019

Add MacOS notarization support (uplift to 0.72.x) #3855

Closed

4 tasks

This was referenced Nov 1, 2019

Add MacOS notarization support (uplift to 0.72.x) #3856

Merged

Add MacOS notarization support (uplift to 0.71.x) #3857

Merged

mbacchi added a commit that referenced this pull request Nov 3, 2019

Uplift of #3725 (squashed) to dev

5089f3c

mbacchi added a commit that referenced this pull request Nov 4, 2019

Uplift of #3725 (squashed) to beta

c52d462

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MacOS notarization support #3725

Add MacOS notarization support #3725

mbacchi commented Oct 16, 2019 •

edited by bsclifton

Loading

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 23, 2019

bridiver Oct 23, 2019

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 23, 2019 •

edited

Loading

bsclifton Oct 29, 2019

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 23, 2019

bridiver Oct 23, 2019

bsclifton Oct 29, 2019 •

edited

Loading

bridiver Oct 31, 2019

bridiver Oct 31, 2019

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 23, 2019

mbacchi Oct 28, 2019

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 23, 2019

bsclifton Oct 29, 2019

bridiver Oct 31, 2019

bridiver Oct 23, 2019

mbacchi Oct 23, 2019

bridiver Oct 31, 2019

bridiver Oct 31, 2019

bsclifton Oct 29, 2019

bsclifton Oct 29, 2019

mbacchi Oct 29, 2019

bsclifton left a comment

Add MacOS notarization support #3725

Add MacOS notarization support #3725

Conversation

mbacchi commented Oct 16, 2019 • edited by bsclifton Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

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

bridiver Oct 23, 2019 • edited Loading

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

bsclifton Oct 29, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

mbacchi commented Oct 16, 2019 •

edited by bsclifton

Loading

bridiver Oct 23, 2019 •

edited

Loading

bsclifton Oct 29, 2019 •

edited

Loading