Skip to content
This repository has been archived by the owner on Jul 23, 2022. It is now read-only.

Ensure safari extension is included in mas and not only darwin #1128

Merged
merged 3 commits into from
Oct 29, 2021

Conversation

Hinton
Copy link
Member

@Hinton Hinton commented Oct 28, 2021

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

The upgrade of electron builder in #1088 changed how we include and sign the safari extension. Unfortunately when building for the mac app store the electronPlatformName is not darwin but mas. This resulted in the extension not being included in the desktop release.

I needed to also change from signApp to sign since we need to sign a MAS application, which required me to replicate some of the logic used to build the arguments.

Code changes

  • scripts/after-sign.js: Ensure mas runs the specific copy and re-sign of the safari extension.

Testing requirements

Verify the GH release pipeline contains the safari.appex in PlugIn directory.

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

cscharf
cscharf previously approved these changes Oct 28, 2021
@Hinton
Copy link
Member Author

Hinton commented Oct 28, 2021

CI build is failing, we'll need to replace signApp with sign and re-build some of the arguments to match what https://github.com/electron-userland/electron-builder/blob/fce1a1fab66e3f5cd741a4cecc4af8377aea9dd8/packages/app-builder-lib/src/macPackager.ts#L170-L172 produces

@sschwetz
Copy link

Once the notarization by apple is successful how long will the wait be for it to appear in the store?

Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

I wish we could get away from doing this.

Side note that doesn't need to be acted on here, the afterSign key can be applied to only mac builds in package.json so we don't need to worry about identifying whether it's a mac build

@kspearrin kspearrin merged commit 05470b4 into master Oct 29, 2021
@kspearrin kspearrin deleted the hotfix/safari-extension branch October 29, 2021 13:48
vgrassia pushed a commit that referenced this pull request Oct 29, 2021
* Ensure safari extension is included in mas and not only darwin

* Add support for re-signing mas

* Add support for mas-dev

(cherry picked from commit 05470b4)
@jslopezgithub
Copy link

I downloaded 1.29.1 and I'm still seeing the same issue. I ran lsregister and rebooted and it did not make a difference.
MacOS 11.6.1 (20G224) / Safari 15.1

@kspearrin
Copy link
Member

kspearrin commented Oct 29, 2021

@jslopezgithub 1.29.1 isn't available on the mac app store yet. It is pending review from Apple.

@jslopezgithub
Copy link

jslopezgithub commented Oct 29, 2021

@kspearrin
Copy link
Member

@jslopezgithub The extension is only available on the Mac App Store version.

@andrewkr
Copy link

andrewkr commented Nov 2, 2021

If I've correctly read what's above, 1.29.1 from the Mac App Store ( first screenshot) should contain the fix for the missing extension.

That's what I've downloaded this afternoon, but as my second screenshot shows, it isn't there.

Screen Shot 2021-11-02 at 15 55 53

Screen Shot 2021-11-02 at 16 02 01

What am I missing? (Yes, apart from the extension.)

@djsmith85
Copy link
Contributor

Hi @andrewkr sorry for the inconvenience, please have a look at https://community.bitwarden.com/t/bitwarden-does-not-show-as-safari-extension-macos/28229/32

@andrewkr
Copy link

andrewkr commented Nov 8, 2021

Hi @andrewkr sorry for the inconvenience, please have a look at https://community.bitwarden.com/t/bitwarden-does-not-show-as-safari-extension-macos/28229/32

Thanks!

It took a few tries, but I got there in the end.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants