Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
LDAP opened this issue Feb 12, 2023 · 4 comments
Open

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

LDAP opened this issue Feb 12, 2023 · 4 comments

Comments

@LDAP
Copy link

LDAP commented Feb 12, 2023

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

Preferred solution:

  • Plugins (LSP-* helpers) that depend on another plugin (LSP) declare that dependency.
  • On update, Package Control unloads all plugins (LSP helpers) that depend on the plugin (LSP) that is updated. Then updates the plugin and then reloads all dependencies.

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.

@LDAP LDAP changed the title [four-point-oh] Unload packages that depend on a plugin that is updated [four-point-oh?] Unload packages that depend on a plugin that is updated Feb 12, 2023
@FichteFoll
Copy link
Collaborator

FichteFoll commented Feb 12, 2023

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:

  • LSP is the base package.
  • LSP-plugin depends on LSP and declares this dependency.
  • PC was used to install both.

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
Copy link

kaste commented Feb 13, 2023

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
Copy link
Collaborator

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
Copy link
Author

LDAP commented Feb 13, 2023

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.

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

No branches or pull requests

4 participants