[MAINT, packaging] Remove support for briefcase installers #5804

jaimergp · 2023-05-04T13:52:04Z

Fixes/Closes

Description

The Briefcase bundles have not been working for a while (at least in some platforms), nobody is testing them either, and our development efforts are mostly directed to the constructor installers.
In this PR, they are removed to avoid CI waste and permit deleting some strange code paths for Briefcase workarounds.

References

Type of change

Removes / deprecates code

How has this been tested?

No CI jobs for Briefcase
Tests pass without the extra Briefcase workarounds

Final checklist:

My PR is the minimum possible work for the desired functionality
I have made corresponding changes to the documentation: Update installation docs to remove briefcase bundle mentions docs#147

for more information, see https://pre-commit.ci

codecov · 2023-05-04T14:30:28Z

Codecov Report

Merging #5804 (5458701) into main (436d983) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

❗ Current head 5458701 differs from pull request most recent head ae412a9. Consider uploading reports for the commit ae412a9 to get more accurate results

@@            Coverage Diff             @@
##             main    #5804      +/-   ##
==========================================
- Coverage   89.86%   89.86%   -0.01%     
==========================================
  Files         614      614              
  Lines       52210    52190      -20     
==========================================
- Hits        46918    46900      -18     
+ Misses       5292     5290       -2

Impacted Files	Coverage Δ
napari/plugins/__init__.py	`93.54% <ø> (-0.74%)`	⬇️
napari/utils/_appdirs.py	`82.14% <0.00%> (+5.47%)`	⬆️
napari/utils/misc.py	`84.41% <ø> (+1.36%)`	⬆️
napari/_qt/dialogs/qt_package_installer.py	`87.35% <100.00%> (+0.28%)`	⬆️
napari/plugins/_plugin_manager.py	`76.08% <100.00%> (+0.01%)`	⬆️

... and 15 files with indirect coverage changes

jaimergp · 2023-05-04T15:00:11Z

napari/utils/_appdirs.py

Many functions on this module are Briefcase-specific adjustments. We might just remove them, but I wonder if we have to follow a deprecation policy for these?

This is a private module. Did they are exposed from non-private ones?

I didn't check so I don't know, but I honestly hope they are not being used by any other project.

Even if other project uses it but importing from a private module then it is not our problem.

AFAIK the only user is the plugin manager dialog which we control fully, so we are good. I removed those functions because they are not needed anymore in the PipInstaller workarounds.

…rounds

…move-briefcase

…or_app() Sometimes the intent is to check whether we are in an installer-provided installation, not to check for Briefcase itself (for technical workarounds, not UX ones).

jaimergp · 2023-05-15T08:28:03Z

Some more changes:

I removed most of _appdirs custom functions because the only usage was the Plugin Manager dialog, where workarounds for Briefcase were implemented. We don't need those anymore!
I removed the usage of running_as_bundled_app() and replaced it with running_as_constructor_app() where necessary, but there are some places were I don't know if the check was done for UX purposes or the necessity of technical workarounds. I'll need feedback here! (Questions inline below).

jaimergp · 2023-05-15T08:28:49Z

napari/_qt/dialogs/qt_package_installer.py

        return (*args, *self.pkgs)

    def environment(
        self, env: QProcessEnvironment = None
    ) -> QProcessEnvironment:
        if env is None:
            env = QProcessEnvironment.systemEnvironment()
-        combined_paths = os.pathsep.join(
-            [
-                user_site_packages(),


This was only used in Briefcase, AFAIK. There's no plugins/ installation directory anywhere else, right?

jaimergp · 2023-05-15T08:29:16Z

napari/_qt/menus/file_menu.py

@@ -123,7 +123,7 @@ def __init__(self, window: 'Window') -> None:
                'shortcut': 'Ctrl+W',
            },
            {
-                'when': running_as_bundled_app(),
+                'when': running_as_constructor_app(),


Is the Restart button a UX feature or something we needed in Briefcase for XYZ reasons?

jaimergp · 2023-05-15T08:31:42Z

napari/_qt/qt_main_window.py


-        if not running_as_bundled_app():
+        if not running_as_constructor_app():
            process.setArguments(sys.argv)


Not sure why this is avoided in bundled apps, but 🤷 We might even want the sys.argv stuff in the constructor apps. We use the python -m napari syntax to launch those, and those scripts are called by specific platform launchers for OS integrations...

The broader question is... do we even need a Restart button now? It was only used with Briefcase, apparently...

jaimergp · 2023-05-15T08:32:47Z

napari/plugins/_plugin_manager.py


-        if sys.platform.startswith('linux') and running_as_bundled_app():
-            sys.path.append(user_site_packages())
-
    def _initialize(self):


Bye to this hack :D user_site_packages() was something like {sys.prefix}/plugins/lib/pythonX.Y/site-packages in Briefcase, I think, but nowhere else!

jaimergp · 2023-05-15T08:33:42Z

napari/utils/misc.py

@@ -51,17 +51,26 @@ def parse_version(v) -> 'packaging.version._BaseVersion':


 def running_as_bundled_app(*, check_conda=True) -> bool:
-    """Infer whether we are running as a briefcase bundle."""
+    """Infer whether we are running as a bundle."""


Now that we are deprecating this function... I guess I need to revert this?

Suggested change

"""Infer whether we are running as a bundle."""

"""Infer whether we are running as a briefcase bundle."""

napari/utils/misc.py

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

jaimergp · 2023-05-19T09:46:28Z

The restart functionality was added in #2540. @goanpeca - do you remember what was the rationale for this feature? Would we need that in constructor-based bundles too?

goanpeca · 2023-05-19T15:28:58Z

Hi @jaimergp, so at the moment some preference options required a restart, so that is why that option was added. I believe with @kcpevey (async preferences) work this will no longer be necessary. However restart will still be required when for example, language settings are updated. Now we can decide if the UI is just "you will need to close and restart" or if we continue to handle it for the user with "click yes to restart" or similar.

Would we need that in constructor-based bundles too?

Would be nice, any difficulties you foresee in that 🤔 ? (I would not call not having that for constructor based installers a blocker)

Czaki · 2023-05-19T15:39:54Z

So why this option is disabled for non-bundle installs?

jaimergp · 2023-05-19T15:44:54Z

It was initially suggested at #2540 (comment), but no clue about the context.

psobolewskiPhD · 2023-05-21T16:40:21Z

Now we can decide if the UI is just "you will need to close and restart" or if we continue to handle it for the user with "click yes to restart" or similar.

We already have a pop-up style message for install/uninstall of npe2 plugins, without a restart button. I think as long as it's clear that the preference requires it then I think quit and relaunch is not a hard/foreign concept.

psobolewskiPhD · 2023-05-22T18:50:47Z

Just to clarify: the restart thing was a Briefcase bundle only thing—which is why I've never seen it?
Looking at the other PR it looks like it's an extra File menu item Restart. I've never seen that in any GUI app I use, so I'd vote to totally remove it and if we want that feature to be a napari feature we can add it back.

Czaki · 2023-05-23T10:24:11Z

Because of the current failures of CI, I would like to merge this and move the discussion about restarting to Issue/followup PR.

@napari/copy could you check it?

psobolewskiPhD · 2023-05-24T16:33:40Z

@Czaki @jaimergp I tweaked the wording a bit, hope it's ok.

# Description The briefcase bundle has been broken and has been removed by napari PR: napari/napari#5804 This PR removes mention of this bundle from the user-facing installation docs. ## Type of change - [X] Removes outdated documentation # References Depends on napari/napari#5804 ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: Kira Evans <contact@kne42.me>

Closes #5506 The Briefcase bundles have not been working for a while (at least in some platforms), nobody is testing them either, and our development efforts are mostly directed to the `constructor` installers. In this PR, they are removed to [avoid CI waste](#5660) and permit deleting some strange code paths for Briefcase workarounds. - [X] Removes / deprecates code  - [x] No CI jobs for Briefcase - [x] Tests pass without the extra Briefcase workarounds - [X] My PR is the minimum possible work for the desired functionality - [x] I have made corresponding changes to the documentation: napari/docs#147 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

# Description The briefcase bundle has been broken and has been removed by napari PR: #5804 This PR removes mention of this bundle from the user-facing installation docs. ## Type of change - [X] Removes outdated documentation # References Depends on #5804 ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: Kira Evans <contact@kne42.me>

Remove support for briefcase installers

76ed248

github-actions bot added qt Relates to qt task labels May 4, 2023

github-actions bot assigned jaimergp May 4, 2023

jaimergp added the maintenance PR with maintance changes, label May 4, 2023

[pre-commit.ci] auto fixes from pre-commit.com hooks

5458701

for more information, see https://pre-commit.ci

jaimergp mentioned this pull request May 4, 2023

Update installation docs to remove briefcase bundle mentions napari/docs#147

Merged

7 tasks

jaimergp changed the title ~~Remove support for briefcase installers~~ Remove support for briefcase installers (WIP) May 4, 2023

jaimergp mentioned this pull request May 4, 2023

Get CI Time under check. Aka CI takes waaayyy too long. #5660

Closed

jaimergp commented May 4, 2023

View reviewed changes

jaimergp marked this pull request as ready for review May 5, 2023 09:43

Merge branch 'main' of github.com:napari/napari into remove-briefcase

86c589c

Czaki mentioned this pull request May 8, 2023

MAINT: Don't test windows-bundle on every PR. #5674

Closed

jaimergp added 3 commits May 15, 2023 10:14

remove unused functions now that we don't support the briefcase worka…

58b9759

…rounds

Merge branch 'remove-briefcase' of github.com:jaimergp/napari into re…

35e2eb3

…move-briefcase

deprecate running_as_bundled_app(), replace with running_as_construct…

c2cbaab

…or_app() Sometimes the intent is to check whether we are in an installer-provided installation, not to check for Briefcase itself (for technical workarounds, not UX ones).

jaimergp commented May 15, 2023

View reviewed changes

Czaki reviewed May 15, 2023

View reviewed changes

napari/utils/misc.py Outdated Show resolved Hide resolved

one more

ae412a9

Czaki added this to the 0.4.18 milestone May 15, 2023

jaimergp commented May 15, 2023

View reviewed changes

napari/utils/misc.py Outdated Show resolved Hide resolved

Apply suggestions from code review

adf14a9

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>

jaimergp changed the title ~~Remove support for briefcase installers (WIP)~~ Remove support for briefcase installers May 15, 2023

Czaki approved these changes May 16, 2023

View reviewed changes

Czaki added the ready to merge Last chance for comments! Will be merged in ~24h label May 16, 2023

psobolewskiPhD mentioned this pull request May 22, 2023

Provide more information when plugin cannot be installed in bundle #4577

Closed

11 tasks

psobolewskiPhD changed the title ~~Remove support for briefcase installers~~ [MAINT, packaging] Remove support for briefcase installers May 24, 2023

Czaki merged commit d4ba4a0 into napari:main May 24, 2023
28 checks passed

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label May 25, 2023

This was referenced Jun 7, 2023

[MAINT] Remove plugin manager source (move to napari/napari-plugin-manager) #5743

Merged

v0.4.18 - release process #5911

Closed

jaimergp mentioned this pull request Jun 12, 2023

Remove briefcase specific workarounds napari/napari-plugin-manager#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT, packaging] Remove support for briefcase installers #5804

[MAINT, packaging] Remove support for briefcase installers #5804

jaimergp commented May 4, 2023 •

edited by psobolewskiPhD

Loading

codecov bot commented May 4, 2023 •

edited

Loading

jaimergp May 4, 2023

Czaki May 11, 2023

jaimergp May 11, 2023

Czaki May 11, 2023

jaimergp May 15, 2023

jaimergp commented May 15, 2023

jaimergp May 15, 2023

jaimergp May 15, 2023

jaimergp May 15, 2023

jaimergp May 15, 2023

jaimergp May 15, 2023

jaimergp commented May 19, 2023

goanpeca commented May 19, 2023

Czaki commented May 19, 2023

jaimergp commented May 19, 2023 •

edited

Loading

psobolewskiPhD commented May 21, 2023

psobolewskiPhD commented May 22, 2023

Czaki commented May 23, 2023

psobolewskiPhD commented May 24, 2023

	"""Infer whether we are running as a bundle."""
	"""Infer whether we are running as a briefcase bundle."""

[MAINT, packaging] Remove support for briefcase installers #5804

[MAINT, packaging] Remove support for briefcase installers #5804

Conversation

jaimergp commented May 4, 2023 • edited by psobolewskiPhD Loading

Fixes/Closes

Description

References

Type of change

How has this been tested?

Final checklist:

codecov bot commented May 4, 2023 • edited Loading

Codecov Report

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

jaimergp commented May 15, 2023

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

jaimergp commented May 19, 2023

goanpeca commented May 19, 2023

Czaki commented May 19, 2023

jaimergp commented May 19, 2023 • edited Loading

psobolewskiPhD commented May 21, 2023

psobolewskiPhD commented May 22, 2023

Czaki commented May 23, 2023

psobolewskiPhD commented May 24, 2023

jaimergp commented May 4, 2023 •

edited by psobolewskiPhD

Loading

codecov bot commented May 4, 2023 •

edited

Loading

jaimergp commented May 19, 2023 •

edited

Loading