wbond / package_control

The Sublime Text package manager
https://packagecontrol.io
4.79k stars 815 forks source link

[four-point-oh?] Unload packages that depend on a plugin that is updated #1628

Open LDAP opened 1 year ago

LDAP commented 1 year ago

When updating a dependency (e.g. LSP) all plugins (e.g. LSP-* helpers) that depend on LSP break.

Preferred solution:

This should fix the issue that ST needs to be restarted after an update. Bonus: LSP does not need explicitly to be installed because Package Control knows about the dependency.

FichteFoll commented 1 year ago

First of all, PC does concern itself with inter-dependecnies across packages at all currently. Packages can only depend on libraries (new term, previously "dependencies"). You touched on that in your post, but I wanted to make it clear.

If we assume that PC knew about a package's dependency packages, it would also need to have deep knowledge about the modules contained in that dependency because in order to properly unload them, PC would need to remove them all from sys.modules to ensure that they do not get imported from the module cache again later. That's already a pretty high border to cross and it happens with updates within the same package as well.

Now, PC could do something similar to what some modular packages already do when being reloaded and just unload any module that starts with the package name as the prefix (example, but that still won't clear up any already existing references to these modules or an attribute (or class instance) from these modules. It would have to rely on other packages correctly declaring their dependencies (and also unloading all of them).

Then we would end up with the following situation:

and the following procedure:

  1. ST is started.
  2. plugins LSP, LSP-plugin, and PC are loaded.
  3. PC detects an update for LSP.
  4. PC disables LSP and all the packages that depend on it (LSP-plugin).
  5. ST unloads the plugin modules, asynchronously and with no feedback to PC itself (plugin modules are modules at the package's root level).
  6. PC deletes all modules that begin with LSP. from sys.modules and does the same with the depending packges (order shouldn't matter).
  7. PC performs the upgrade of LSP by replacing its .sublime-package file or the Package folders' contents.
  8. PC re-enables LSP and LSP-plugin
  9. ST loads the plugin cleanly

That sounds like it should work but can definitely be fragile (e.g. if something registers modules outside of their prefixed namespace by modifying sys.path) but may be enough for the 99% case (and sys.path should be avoided regardless). Also, PC could apply the same for library updates as well, again assuming that Packages declare their dependencies properly.

kaste commented 1 year ago

Does PC any of this already? I thought it just just disables/enables the package, but doesn't touch e.g. sys.modules. (So hot-reloading would out-of-the-current-box/state-of-the-current-art for the simplest possible plugins. Even GitSavvy doesn't work and ST must be restarted.)

Just to be clear what the starting point is.

deathaxe commented 1 year ago

No, PC doesn't do anything of that atm, even not in four-point-oh branch.

  1. Clearing sys.modules was considered but dropped for the moment due to experiences made with how it would cause trouble with packages such as LSP.

    GitSavvy maybe another candidate, but I think it fails reloading just because of the integrated AutomaticPackageReloader hack. Most packages should reload fine, if sys.modules is cleared out before importing anything new, except those which other packages depend on.

  2. There's also no concept for a dependency resolver on package level.

    dependencies.json would need to be extended so it can also contain package dependency definitions. It supports libraries only, atm. The repository.json scheme would need to be extended to enable packages to define dependencies upstream in the same way as it is possible for libraries.

    Much effort was taken to disable and reenable all installed/updated/removed packages in one step to avoid possible race conditions. Adding package level dependencies would require a further (massive?) refactoring of the whole "job batching" strategy.

LDAP commented 1 year ago

Maybe this can be done dynamically without explicitly specifying the dependencies. I experimented a bit with MetaPathFinder and I believe by using something like this (similar for 3.8):

import sys
from importlib.abc import MetaPathFinder

PACKAGE_DEPENDENCIES = {}

class Python33DependencyDetector(MetaPathFinder):

    def find_module(self, fullname, path):
        # Detect which package does import 'fullname' and add to PACKAGE_DEPENDENCIES
        return None

current_finder = Python33DependencyDetector()
# Insert at index 0 to intecept loading procedure
sys.meta_path.insert(0, current_finder)
print(sys.meta_path)

def plugin_unloaded():
    global current_finder
    assert current_finder
    sys.meta_path.remove(current_finder)
    current_finder = None

the packages that depend on other packages can be detected by walking the call stack.