Replace previous binary with n-api native module

[csett86]

This is preparation work for M1 and universal binary support

[csett86]

Apart from that it should fix #5 as it is again part of the normal notarization and checking during runtime on Big Sur.

[csett86]

FYI, I tested this via example.js and it worked fine on Big Sur (x64).

[saghul]

Very nice work, hope this gets merged soon!

[csett86]

Hi @karaggeorge

I kindly ask you to review this pull request. If there is anything I can improve, please suggest (eg I can squash all the commits into one and reset the branch on my repo if you like)

Kind regards, Christoph

[sindresorhus]

While I do think using N-API is the right choice here. Out of curiosity, is it actually needed for M1 support?

[csett86]

Regarding the question why this is necessary for M1 support: Short summary: It is not strictly necessary, but it makes it a lot more easier.

The module before also contained native code, but without node / node-gyp being aware of it and how to rebuild it. This meant, that the native code was not compiled for M1 / arm64 when eg. electron-builder was updated to support M1 (https://github.com/electron-userland/electron-builder/pull/5426). With the new binding.gyp and install script tools like electron-builder will pick this up as a dependency that contains native code and will automatically rebuild it for the specified arch, including arm64.

When later electron-builder was updated for universal builds (https://github.com/electron-userland/electron-builder/pull/5481), the same applies. In addition, this time another shortcoming of the previous solution (native code just dropped as a binary but node not being aware) came to light: electron-builder noticed that there is a binary inside and that it could not be appended to the app.asar archive but instead had to be placed into the folder app.asar.unpacked. This in turn gets more difficult if that has to be present in multiple architectures, as then electron/universal will create app-x64.asar.unpacked and app-arm64.asar.unpacked, which breaks assumptions done in eg. fixPathForAsarUnpack to locate the binary.

All of this hassle is removed, if the native code uses either N-API or the other native code APIs described in https://nodejs.org/api/addons.html. I chose N-API, as this seemed the most stable way forward (as we only need to pass a bool back anyway).

I hope this explains the reasons behind this pull request. I have tested the approach already with my fork https://github.com/csett86/mac-screen-capture-permissions which I included in releases of https://github.com/csett86/jitsi-meet-electron.

Kind regards, Christoph

[sindresorhus]

Thanks for elaborating. That makes sense.

[sindresorhus]

I just noticed that macOS 10.15 or later actually has proper APIs for this. We should use those when on macOS 10.15 or later: https://stackoverflow.com/a/65423834/64949

[csett86]

The official apis are a lot easier than the implicit checks that were done before, agree. If you prefer I can directly rewrite the check to the official API as part of this PR

[sindresorhus]

Yes, that would be great. We still need to preserve the old check for older macOS versions though.

[csett86]

The check anyway return always true for mac os versions lower than 10.15, see https://github.com/karaggeorge/mac-screen-capture-permissions/blob/master/index.js#L10, and with 10.15 the mentioned check api is available. With that I dont see that anything native needs to be preserved for older mac os versions.

[sindresorhus]

👍🏻

[csett86]

@sindresorhus proper APIs now used, please review

[sindresorhus]

I haven't actually tested this, but the code looks good to me.

// @karaggeorge

[csett86]

Turns out the official API crashes on 10.15 with:

Dyld Error Message:
Symbol not found: _CGPreflightScreenCaptureAccess
Expected in: flat namespace
Termination Reason:    DYLD, [0x4] Symbol missing

See also: https://github.com/lwouis/alt-tab-macos/issues/704#issuecomment-762852689

Even though its officially marked as supported in the docs for 10.15+: https://developer.apple.com/documentation/coregraphics/3656523-cgpreflightscreencaptureaccess?language=objc

So I will revert this pull request to the previous method (that was also used in the swift implementation before here)

[karaggeorge]

Hey @csett86 thank you for all the work and the detailed explanation. I'll pull this and run it locally to check

[csett86]

Hey @karaggeorge thanks a lot, I have tested and shipped the code already with the forks https://github.com/freifunkMUC/jitsi-meet-electron and https://github.com/csett86/jitsi-meet-electron

So looking forward to get this back into the original and released so that I can revert back to the original of this module.

If you have any further suggestions, please let me know.

[sindresorhus]

So I will revert this pull request to the previous method (that was also used in the swift implementation before here)

Can you use the modern method on macOS 11 and the old hack on macOS 10.15?

[csett86]

@sindresorhus Now both APIs are part of the pull request, the modern method for 11+ and the old hack as "legacy" for 10.15 only.

[csett86]

@sindresorhus Thanks for updating macos-version, I have used it and now updated the index.js as suggested.

[L3o-pold]

Notarization seems broken again, but worked with the version used the 19th January. Even if I rollback I can't notarize correctly for macOS 11+.

[csett86]

@L3o-pold notarization works for me with this pull request and macOS 11.2. As an example, see https://github.com/csett86/jitsi-meet-electron/releases/tag/v2.5.2-rc1 where the version of this pull request (e6697f46a200c31737425727866757b737594985) is used:

[L3o-pold]

@csett86 you are right, something strange happens with the notarization of my app.

[csett86]

Hi @karaggeorge, looking forward if you could take a look at this pull request. The code is tested already in https://github.com/csett86/jitsi-meet-electron/releases/tag/v2.5.3 but I would like to upstream it.

[karaggeorge]

@csett86 sorry for the delay, have been very busy at my day job lately. I appreciate all the work you've done. I'll merge this now, and release it in a few hours

[karaggeorge]

I'm having trouble building this on MacOS 10.15 and I'm getting an error invalid version number in '-mmacosx-version-min=11.0'

Which doesn't let me get the executable for legacy either.

I've installed the latest xcode command line tools I could (12.4). I should be able to build this right?

[csett86]

Yes, I also built the module with macos 10.15, as this is the only version currently available as GA for github actions.

Here the built of mac-screen-capture-permissions/ on its own for x64: https://github.com/csett86/mac-screen-capture-permissions/runs/1973130241?check_suite_focus=true#step:4:1

Here rebuilt for x64 and arm as part of jitsi-meet-electron: https://github.com/csett86/jitsi-meet-electron/runs/1964045932?check_suite_focus=true#step:7:229

[saghul]

This is breaking for me:

> mac-screen-capture-permissions@1.1.0 native_build /Users/saghul/work/jitsi/jitsi-meet-electron-utils/node_modules/mac-screen-capture-permissions
> node-gyp rebuild

  CXX(target) Release/obj.target/screencapturepermissions/screen-capture-permissions.o
../screen-capture-permissions.m:10:20: error: implicit declaration of function 'CGPreflightScreenCaptureAccess' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  hasPermissions = CGPreflightScreenCaptureAccess();
                   ^
1 error generated.
make: *** [Release/obj.target/screencapturepermissions/screen-capture-permissions.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/Cellar/node@12/12.20.2/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (events.js:314:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12)
gyp ERR! System Darwin 19.6.0
gyp ERR! command "/usr/local/Cellar/node@12/12.20.2/bin/node" "/usr/local/Cellar/node@12/12.20.2/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/saghul/work/jitsi/jitsi-meet-electron-utils/node_modules/mac-screen-capture-permissions
gyp ERR! node -v v12.20.2
gyp ERR! node-gyp -v v5.1.0
gyp ERR! not ok
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! mac-screen-capture-permissions@1.1.0 native_build: `node-gyp rebuild`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the mac-screen-capture-permissions@1.1.0 native_build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/saghul/.npm/_logs/2021-02-25T08_57_43_596Z-debug.log

[saghul]

Maybe CoreGraphics needs to be imported?

[csett86]

@saghul the module needs to be built with macOS 11+ SDK, which then includes CGPreflightScreenCaptureAccess in CoreGraphics, which is included by Foundation. This is done when building on 10.15 with https://github.com/csett86/mac-screen-capture-permissions/blob/master/.github/workflows/node.js.yml#L20

The resulting module still runs fine on 10.15 and 11+, I have tested that with https://github.com/csett86/jitsi-meet-electron/releases/tag/v2.5.4

I will propose a disclaimer to the README as a pull request

[saghul]

I'm on 10.15 with Xcode 12.4, that should have the 11.0 SDK, shouldn't it?

More importantly, what does the new API provide? I looked around it seems like it's not that great because it returns the value when the application starts but doesn't update thereafter.

[csett86]

@saghul The new API can probe whether the application has been granted screenshare permission without prompting the user to immediately allow screensharing permission for the application. In combination with jitsi-meet-electron this has the advantage that the user is not prompted on every startup of the app to allow screensharing if the user does not use the screensharing feature. The user is then correctly prompted when using the screensharing feature for the first time.

And yes, 10.15 with Xcode 12.4 has the 11.0 SDK. Thats how I build it in github actions as well, as github actions runner macos-latest is still 10.15 on github (despite the name).

Action: https://github.com/csett86/mac-screen-capture-permissions/blob/master/.github/workflows/node.js.yml and https://github.com/actions/virtual-environments#available-environments for what macos-latest means.

[csett86]

@saghul and I quickly checked, the API returns the current value and updates if you change the permission in system settings. See below for a quick PoC which I hacked to together based on the included example.js and then removing the permissions in system settings and the value changes to false:

Same works the other way round from false to true:

[saghul]

Thanks for being so thorough!

I wonder why I get the build failure though...

[csett86]

@saghul I can just guess now, but it seems that your build is still not using the macOS 11 SDK and thus failing. Try this when building this library stand-alone:

export SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk
npm run native_build

or when building jitsi-meet-electron:

export SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk
npm run dist

[csett86]

btw, next Xcode 12.5 release will require macOS 11+ anyway: https://xcodereleases.com/

[saghul]

Well I should need to export SDK_ROOT really. And I don't think I'll be updating to Big Sur just because of this, so alas this means I cannot use the current version of the package.

To me it seems like a build flag is missing. In addition, this can probably be done as a single module, with a runtime availability check, instead of having to build 2 modules.

I'll take a stab at it when I find some time, if nobody beats me to it ;-)

[csett86]

I tried doing it in a single module, but when loading a binary with reference to CGPreflightScreenCaptureAccess in it (even without being called), in my attempts that module could not be linked during runtime on 10.15 and thus failed to load. Thats why I settled for 2 modules to load in the end.

But more than happy to see if this can be avoided, I am not too proud of the whole duplication necessary with the 2 modules.

Btw, to build you need to export SDKROOT, not SDK_ROOT.

[saghul]

I figured it out. Building on Catalina with the CLT instead of Xcode just won't work.

Here is PR simplifying the code to use a single addon: https://github.com/karaggeorge/mac-screen-capture-permissions/pull/8