Feat/mac sandbox pre release pyinstaller #7803

toofar · 2023-07-27T08:59:12Z

Switch to PyInstaller develop branch to get v6.0 changes which brings:

WebEngine/Chromium sandboxing for macOS builds
Removes the need for our downstream patching of the app on macOS
Unifies resource lookups to be via importlib_resources (this is actually possible as of 2021 but we hadn't picked up on it)

This is a pretty standard adaptation to a new major version of a dependency except in this case the major version isn't released yet, so I'm pointing at a specific commit on their develop branch (the latest commit as of today).

In my opinion we probably want to do a little more testing of the resulting binaries before committing to this. Although I don't think the testing surface area is huge? Like it runs, resource files can be loaded, it can be signed? There are binaries here, based of this test branch with a custom CI job.

Potential testing checklist:

installs over an old version with no warnings
it launches
can prompt to capture audio or location
- test with jitsi meet
:help is rendered like normal
userscripts work (if packaged?)
pdf-js works
- https://www.ipcc.ch/site/assets/uploads/2021/06/Fact_sheet_AR6.pdf
- or qute://pdfjs/build/pdf.js on builds with 5.15.2
application icon on shortcut and taskbar is correct
extensions are loaded
- the reload command is registered

TODO:

open issue to track reverting the temp stuff for the pre-release pyinstaller once a release comes out (requirements files and PYINSTALLER_COMPILE_BOOTLOADER) ✔️ Switch back to released PyInstaller #7806
probably move the PYINSTALLER_COMPILE_BOOTLOADER=true out of tox into the build script so that it gets set for local builds too ✔️
do manual testing I guess. I went through a few earlier, but not all of them. ✔️
remove the warning page and the place that triggers it qutebrowser/html/warning-sandboxing.html ✔️
apologies to @rokm for pulling in the develop branch

Should I, GitHub, should I? 🤔

Testing (it seems userscripts are not packaged):

Windows (10)

installs over an old version with no warnings
it launches
can prompt to capture audio or location
* tested with jitsi meet
:help is rendered like normal
pdf-js works
* https://www.ipcc.ch/site/assets/uploads/2021/06/Fact_sheet_AR6.pdf
application icon on shortcut and taskbar is correct
extensions are loaded
* the reload command is registered

macOS (11.7.8)

installs over an old version with no warnings
it launches
can prompt to capture audio or location
* tested with jitsi meet
:help is rendered like normal
pdf-js works
* https://www.ipcc.ch/site/assets/uploads/2021/06/Fact_sheet_AR6.pdf
application icon on shortcut and taskbar is correct
extensions are loaded
* the reload command is registered

Closes: #7278

PyInstaller has [recently][symlinks] landed a change that should restore the webengine sandbox on macOS builds. With a bit more testing we are hoping that we can go ahead releasing builds based on that in-development PyInstaller codebase even before they've made a release. This commit pins our pyinstaller dependency to be the latest commit on their develop branch. [symlinks]: pyinstaller/pyinstaller#7619

Since we are pulling down PyInstaller off of the develop branch we need to recompile the bootloader, because upstream only commits a new one back to the branch on releases. Luckily all the compiler requirements seem to already be installed on CI. For the record the macOS CI is currently failing with dlopen: dlopen(/Users/runner/work/qutebrowser/qutebrowser/dist/qutebrowser.app/Contents/MacOS/libpython3.10.dylib, 10): image not found And upon inspection of dist/ that file seems to be at ./qutebrowser.app/Contents/Resources/libpython3.10.dylib ./qutebrowser.app/Contents/Frameworks/libpython3.10.dylib ./qutebrowser/_internal/libpython3.10.dylib

pyinstaller patches importlib_resources now so it should transparently handle this special case for us. ref: https://pyinstaller.org/en/stable/CHANGES.html?highlight=importlib.resources#id94

Not that we are looking up resources via importlib_resources for pyinstaller builds too we need to change where the data files are installed to to match what importlib_resources is expecting. There was a comment in the previous resource lookup special case complaining about the data files being at the top level so it seems this is a change for the better anyhow. Observed paths: requested file: qutebrowser.app/Contents/Frameworks/qutebrowser/config/configdata.yml actual file : qutebrowser.app/Contents/Frameworks/config/configdata.yml

misc/requirements/requirements-pyinstaller.txt-raw

scripts/dev/build_release.py

In the past various workarounds have been put in place to move/copy/symlink files in the macOS app build to make PyQt work and let us sign it. As of pyinstaller/pyinstaller#7619 our downstream patching should not be required. The application seems to run fine. The app size is 155 MB. Signing is still to be verified.

toofar · 2023-07-30T03:23:26Z

I've pushed again to:

remove the re-signing
move the PYINSTALLER_COMPILE_BOOTLOADER env var setting into build_release instead of tox.ini so that if you run build_release not via tox you don't have to remember to set it. It's a bit ugly, and while we are going to remove that stuff after the new pyinstaller release comes out, let me know if you do manual builds via tox anyway and I might move it back to how it was before

While looking through the spec file I saw this note and that the comment it refers to was removed a couple of years ago in pyinstaller/pyinstaller@af23dcceb3f997bda40. So it might not be needed anymore.

I'll update the PR description with potential testing notes based off of looking through the spec file.

rokm · 2023-07-30T10:51:43Z

While looking through the spec file I saw this note and that the comment it refers to was removed a couple of years ago in pyinstaller/pyinstaller@af23dcceb3f997bda40. So it might not be needed anymore.

This looks like a work-around for PyInstaller butchering the QtWebEngineCore.framework bundle.

It should be safe to replace that ID with your actual bundle ID now that the .framework bundles are preserved.

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

toofar · 2023-08-05T04:05:48Z

I updated the bundle ID and went through my manual testing stuff and everything seemed to work 🎉. Although I'm not sure how I'm supposed to reproduce the original failure, did it just stop the app from starting?

Also realized there is the macOS sandbox warning page which is probably unneeded now? Or at least should have references to the PyInstaller issue removed.

More interestingly the extensions are no longer being loaded on macOS. Resulting in messages like: "scroll-to-perc: no such command" and "reload: no such command".

>>> from qutebrowser.extensions import loader
>>> list(loader.walk_components())
[]

>> components.__path__
['/Applications/qutebrowser.app/Contents/Frameworks/qutebrowser/components']
>> components.__name__
'qutebrowser.components'

>> pkgutil.iter_importers('qutebrowser.components')
[FileFinder(...)]

However /Applications/qutebrowser.app/Contents/Frameworks/qutebrowser/components is not a real path (the parent folder does exist, it's just got data files in it though).

The workaround of using the PyInstaller importer toc manually (removed in dd2fc8e) seems like it'll still work though:

>>> {p for p in list(pkgutil.iter_importers('qutebrowser'))[2].toc if p.startswith('qutebrowser.components')}
{'qutebrowser.components',
'qutebrowser.components.adblockcommands',
'qutebrowser.components.braveadblock',
'qutebrowser.components.caretcommands',
'qutebrowser.components.hostblock',
'qutebrowser.components.misccommands',
'qutebrowser.components.readlinecommands',
'qutebrowser.components.scrollcommands',
'qutebrowser.components.utils',
'qutebrowser.components.utils.blockutils',
'qutebrowser.components.zoomcommands'}

Restored from dd2fc8e Now that we are putting our data files in the qutebrowser/ directory again pkgutil/importlib is getting confused by that dir existing and returning us a FileLoader for `qutebrowser.components`, I think that's what's happening anyway. Should try reverting that and this commit and see if extensions get loaded right again. So bring back this workaround of using the toc on the PyInstaller loader to get the list of component modules for now. ref: #7803

Now that we aren't patching the build we shouldn't have to try to re-sign in.

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

Restored from dd2fc8e Now that we are putting our data files in the qutebrowser/ directory again pkgutil/importlib is getting confused by that dir existing and returning us a FileLoader for `qutebrowser.components`, I think that's what's happening anyway. Should try reverting that and this commit and see if extensions get loaded right again. So bring back this workaround of using the toc on the PyInstaller loader to get the list of component modules for now. ref: #7803

This reverts commit 8715263. ref: #7803

Restored from dd2fc8e Now that we are putting our data files in the qutebrowser/ directory again pkgutil/importlib is getting confused by that dir existing and returning us a FileLoader for `qutebrowser.components`, I think that's what's happening anyway. Should try reverting that and this commit and see if extensions get loaded right again. So bring back this workaround of using the toc on the PyInstaller loader to get the list of component modules for now. ref: #7803

This reverts commit 8715263. ref: #7803

The-Compiler · 2023-08-08T22:30:40Z

Thanks for all the great work, this definitely gets us a big step closer! <3

I've pinned this to a specific commit in both requirements files for now.

Works for me.

While looking through the spec file I saw this note and that the comment it refers to was removed a couple of years ago in pyinstaller/pyinstaller@af23dcceb3f997bda40. So it might not be needed anymore.

Nice catch!

let me know if you do manual builds via tox anyway and I might move it back to how it was before

I do indeed to ensure I have a consistent environment with everything it needs. So I suppose dd86483 and 8a327be could be reverted?

Although I'm not sure how I'm supposed to reproduce the original failure, did it just stop the app from starting?

I believe renderer processes just didn't start, yeah.

Also realized there is the macOS sandbox warning page which is probably unneeded now? Or at least should have references to the PyInstaller issue removed.

Yep, reverting that was the right call IMHO!

More interestingly the extensions are no longer being loaded on macOS. Resulting in messages like: "scroll-to-perc: no such command" and "reload: no such command".

That's weird! Might be good to try and see if we can get a more minimal reproducer to report this as a PyInstaller regression? pyinstaller/pyinstaller#5959 comes with a testcase which hasn't had any changes and still seems to pass, so I'm not sure what it is that we do differently?

I don't want to deal with having to review development changes of pyinstaller every week. So pin to one commit for now that we can actually test. I'm subscribed to release notifications on github so I'll manually change this back to point to the pyinstaller pypi package ones it does.

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

Restored from dd2fc8e Now that we are putting our data files in the qutebrowser/ directory again pkgutil/importlib is getting confused by that dir existing and returning us a FileLoader for `qutebrowser.components`, I think that's what's happening anyway. Should try reverting that and this commit and see if extensions get loaded right again. So bring back this workaround of using the toc on the PyInstaller loader to get the list of component modules for now. ref: #7803

This reverts commit 8715263. ref: #7803

…e_release_pyinstaller Only conflict was the removal of support for 32bit builds in build_release.py

toofar · 2023-08-12T05:41:50Z

Alright, last call for comments on this one! Latest changes:

I added a Windows 10 minimum version check to the installer
- based on @bitraid's comment here New Windows installer #7315 (comment)
- but with an added conditional to do the old check for the Qt5 installer
- and I naughtily bumped the Qt6 OS requirements down a bit from Win 10 build 1809 -> build 1607 based on our testing. This is lower than the Qt advice but seems to work. Easier enough to increase it again if someone finds an issue?
dropped the two ugly commits around moving the pyinstaller bootload compiling stuff out of tox into the build_release script
adaptions from Drop 32bit Windows release support #7804
I added the Qt5 builds back to my test CI job and actually tested them too
- latest binaries are here: https://github.com/qutebrowser/qutebrowser/actions/runs/5839589092
- I had one fright where installing the windows Qt5 build over top of the Qt6 build causes the application to fail to launch with a stack trace about failing to import PyQt6.QtCore. It seems there was a leftover, empty, _internal\PyQt6\Qt6\bin\locales folder left in the install dir that was tricking the autoselect logic. I expect doing an uninstall first would resolve this, as that should remove the whole install dir
- pdfjs doesn't work on the binary Qt5 version we are packaging, as probably noted elsewhere. Changed the testing instructions to just check that qute://pdfjs/build/pdf.js returns JS
I didn't update scripts/dev/download_release.sh, easy enough to do that during the release

Too old win10 warning (before I dropped the requirement lower):

Error dialog on launch after installing the Qt5 version over the Qt6 version:

Testing the autoselect logic with the above issue (after starting with --qt-wrapper PyQt5):

Tomorrow I'll look at doing a new test case for pyinstaller and move to main my changes around getting better smoke test error logs, but that's all the stuff I have planned for this PR.

The-Compiler · 2023-08-12T09:21:30Z

misc/nsis/qutebrowser.nsi

@bitraid could you give this a look?

misc/nsis/install.nsh

The-Compiler · 2023-08-12T09:23:11Z

misc/requirements/requirements-pyinstaller.txt-raw

@@ -1 +1 @@
-PyInstaller
+pyinstaller @ git+https://github.com/pyinstaller/pyinstaller.git@79f62ef29822169ae00cd4271390d0e3175476ad


If you plan on doing another round of testing with your latest changes, maybe worth bumping up this again? If you'd rather not, I'm fine with that too.

I've done more testing but I don't plan on updating the commit. I've been checking in on subsequent commits and I don't see anything relevant to us.

The-Compiler · 2023-08-12T09:23:37Z

qutebrowser/extensions/loader.py

+def _walk_pyinstaller() -> Iterator[ExtensionInfo]:
+    """Walk extensions when using PyInstaller.
+
+    See https://github.com/pyinstaller/pyinstaller/issues/1905


Note to self, we still should try to figure out whether this has regressed.

I don't think it has, but I'm not sure if we can test it on an old version because of the workarounds we had to do symlinking stuff around. I suppose this might be an issue with the upstream fix for that stuff breaking this use case.

This only started happening with c0d31d3 where the data files changed to be under /Frameworks/qutebrowser/. That's the same path that the python files are reported to be under so the issue is that somehow the /Frameworks/qutebrowser/ folder existing is causing importlib to use the file loader instead of the frozen ones. But it seems to work fine on Windows (TODO: triple check with https://github.com/qutebrowser/qutebrowser/actions/runs/5846907490), so it might be some mac specific workaround going wonky.

I tried to make an upstream test fail here, but it didn't. I think the script in that test as the right structure (a folder with data files at the same level as a folder with python files). I might switch tack and try to do a minimal reproducer based on qutebrowser (including spec file).

Anyway, I don't intend to treat this oddity as blocking (I really want to see all the nightly tests working again!). So I'll probably merge this PR tomorrow.

This might be fixed by pyinstaller/pyinstaller#7885.

Looks like that fixes it indeed. Data files and dynamic modules are both being loaded fine with no special casing for frozen builds from us! Yet another issue that gets solved with me doing nothing but procrastinating 😂

I just did a quick build to check. Will double check in more detail tomorrow.

The-Compiler · 2023-08-12T09:26:22Z

As for the "empty PyQt6 dir" thing: I've seen that too, and considered changing the logic to account for that - but then decided this is one of the "user's environment is in a weird inconsistent thing" situations I'd much rather report to the user rather than just painting over it and assuming something.

misc/nsis/qutebrowser.nsi

The Qt docs for 6.5 say that the minimum supported version is Windows 10 1809. Experimentally it seems qutebrowser and it's dependencies work fine on a version as early 1607. There should be no change in OS version requirements for the Qt5 build, although we've dropped 32 bit support already and in a future version of the installer we may bring the minimum OS version support in line with the Qt6 requirements for simplicity too. Added a new QT5 version into the NSIS scripts so we can do the different version check per installer build. It just uses the python bool serialization format so should always be "True" or "False", but I've added a fallback anyway for consistency.

We dropped 32bit support in #7804 and as a result removed the arch suffix from the binary that pyinstaller produces. This commit removes it form the lookup path in the installer too. Note that we are leaving the arch string in the installer itself for now. Mostly because it'll be removed as part of a later change when the installer itself is refreshed. But it might also be useful to clarify in the installer names what the arch is? Maybe, that reasoning might not fit with the previous change to remove the arch strings.

The CheckPlatform macro will prompt the user user to use the 32bit installer if they are on a 32bit system. But we don't provide a 32bit installer anymore. This commit changes the OS version check for Qt5 builds to be based on checking version numbers ourselves too, so that we can have our own error message. Also moves the Qt5 conditionals to be compile time ones.

toofar added 4 commits July 27, 2023 19:08

Drop 'frozen' special case for resources lookup

f254680

pyinstaller patches importlib_resources now so it should transparently handle this special case for us. ref: https://pyinstaller.org/en/stable/CHANGES.html?highlight=importlib.resources#id94

toofar commented Jul 27, 2023

View reviewed changes

misc/requirements/requirements-pyinstaller.txt-raw Outdated Show resolved Hide resolved

The-Compiler requested changes Jul 27, 2023

View reviewed changes

scripts/dev/build_release.py Outdated Show resolved Hide resolved

scripts/dev/build_release.py Outdated Show resolved Hide resolved

toofar force-pushed the feat/mac_sandbox_pre_release_pyinstaller branch from 92fb2d9 to 4ff4b70 Compare July 30, 2023 02:56

toofar mentioned this pull request Jul 30, 2023

Switch back to released PyInstaller #7806

Closed

toofar added a commit that referenced this pull request Aug 2, 2023

macOS: switch to more suitable bundle ID

ef4fa44

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

toofar added 2 commits August 5, 2023 17:03

Remove local re-signing of macOS build.

c209264

Now that we aren't patching the build we shouldn't have to try to re-sign in.

Remove macOS exception for "sandbox disabled" error message

d084df8

toofar added a commit that referenced this pull request Aug 5, 2023

macOS: switch to more suitable bundle ID

79743df

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

toofar added a commit that referenced this pull request Aug 5, 2023

Revert "qt6: Add macOS sandboxing warning"

906d5b4

This reverts commit 8715263. ref: #7803

toofar force-pushed the feat/mac_sandbox_pre_release_pyinstaller branch from 2a5e5b7 to 906d5b4 Compare August 5, 2023 05:05

toofar added a commit that referenced this pull request Aug 5, 2023

Revert "qt6: Add macOS sandboxing warning"

e468766

This reverts commit 8715263. ref: #7803

toofar force-pushed the feat/mac_sandbox_pre_release_pyinstaller branch from 906d5b4 to e468766 Compare August 5, 2023 07:27

The-Compiler added this to Focus in Pull request backlog Aug 8, 2023

toofar added 5 commits August 12, 2023 13:36

macOS: switch to more suitable bundle ID

3247068

Trying to switch the bundle ID again and see if that makes things work. Will need to check 5.15 too. ref: #7803 (comment) Closes: #5180

Revert "qt6: Add macOS sandboxing warning"

b018f3f

This reverts commit 8715263. ref: #7803

Merge remote-tracking branch 'upstream/main' into feat/mac_sandbox_pr…

0f2d346

…e_release_pyinstaller Only conflict was the removal of support for 32bit builds in build_release.py

toofar force-pushed the feat/mac_sandbox_pre_release_pyinstaller branch from e468766 to 98e0a37 Compare August 12, 2023 05:19

The-Compiler reviewed Aug 12, 2023

View reviewed changes

bitraid reviewed Aug 12, 2023

View reviewed changes

misc/nsis/qutebrowser.nsi Outdated Show resolved Hide resolved

bitraid and others added 4 commits August 13, 2023 15:14

nsis: allow Win10 versions back to 1607

6f7b76f

toofar force-pushed the feat/mac_sandbox_pre_release_pyinstaller branch from 98e0a37 to 7599dbc Compare August 13, 2023 03:15

toofar merged commit 65750e4 into main Aug 14, 2023
60 of 65 checks passed

Pull request backlog automation moved this from Focus to Done Aug 14, 2023

toofar deleted the feat/mac_sandbox_pre_release_pyinstaller branch August 14, 2023 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/mac sandbox pre release pyinstaller #7803

Feat/mac sandbox pre release pyinstaller #7803

toofar commented Jul 27, 2023 •

edited

toofar commented Jul 30, 2023

rokm commented Jul 30, 2023 •

edited

toofar commented Aug 5, 2023

The-Compiler commented Aug 8, 2023

toofar commented Aug 12, 2023 •

edited

The-Compiler Aug 12, 2023

The-Compiler Aug 12, 2023

toofar Aug 13, 2023

The-Compiler Aug 12, 2023

toofar Aug 13, 2023

rokm Aug 25, 2023

toofar Aug 25, 2023

The-Compiler commented Aug 12, 2023

		@@ -1 +1 @@
		PyInstaller
		pyinstaller @ git+https://github.com/pyinstaller/pyinstaller.git@79f62ef29822169ae00cd4271390d0e3175476ad

Feat/mac sandbox pre release pyinstaller #7803

Feat/mac sandbox pre release pyinstaller #7803

Conversation

toofar commented Jul 27, 2023 • edited

toofar commented Jul 30, 2023

rokm commented Jul 30, 2023 • edited

toofar commented Aug 5, 2023

The-Compiler commented Aug 8, 2023

toofar commented Aug 12, 2023 • edited

The-Compiler Aug 12, 2023

Choose a reason for hiding this comment

The-Compiler Aug 12, 2023

Choose a reason for hiding this comment

toofar Aug 13, 2023

Choose a reason for hiding this comment

The-Compiler Aug 12, 2023

Choose a reason for hiding this comment

toofar Aug 13, 2023

Choose a reason for hiding this comment

rokm Aug 25, 2023

Choose a reason for hiding this comment

toofar Aug 25, 2023

Choose a reason for hiding this comment

The-Compiler commented Aug 12, 2023

toofar commented Jul 27, 2023 •

edited

rokm commented Jul 30, 2023 •

edited

toofar commented Aug 12, 2023 •

edited