Skip to content

Conversation

@kaste
Copy link
Contributor

@kaste kaste commented May 30, 2025

PC has the notion of "dependent" or additional packages. These are defined in a standard "Package Control.sublime-settings" file, hosted by the package. (I.e. similar to the "dependencies.json".)

Installing such a package did not install the additional packages in one go, neither did it notify about the incomplete install. In the end, a restart is enough and required to trigger install_missing_packages() to catch up and install everything.

We have two options here. (1) to bail out install_package with
return None and a message to tell the user to restart Sublime, or (2)
to install the packages in a recursive style.

Here we choose (1) which is a simple babystep for a better UX. (2) is
not possible as all install side-effects need to run serialized.

@kaste
Copy link
Contributor Author

kaste commented May 30, 2025

This is a KISS commit and approach. The too long install_package gets even longer.

Since you're using RLocks using a recursive style seems legit. A flat style would be probably preferable. For this we had to return a result object: True, False, None, plus additonal packages and libraries (aka "more work") to be done to the caller (of install_package). But run_install_tasks is in a busy loop and cannot extend its unit of work ("tasks") easily: within the loop we can't call disable_packages again.

I don't even think that this is right refactoring because ideally and imo we would download the zip before the install phase, t.i. before entering install_package, and then would already know which libraries and additional packages we'd expect via reading the zip contents. T.i. we had a complete unit of work before we start the install.

@deathaxe
Copy link
Contributor

Providing dependent packages is not the purpose of collated "installed_packages" accross all available Package Control.sublime-settings.

Too lazy to search for the issue ticket, but it implements a feature request to ship a corporate package with predefined packages to install at first startup of Sublime Text. This package is shipped together with ST out-of-the-box.

It could be abused to auto-install dependent packages, but with described limitations - and is therefore not advertised.

A real mechanism to install and update dependent packages would require more work than just that as

  1. all depending packages must recursively be disabled before updating
  2. and dependencies need to be resovled recursively,
  3. all depending packages must be re-enabled after update

I actually don't plan to extend this feature for dependencies as it abuses "installed_packages".

PC has the notion of "dependent" or additional packages.  These are
defined in a standard "Package Control.sublime-settings" file, hosted
by the package.  (I.e. similar to the "dependencies.json".)

Installing such a package did not install the additional packages in one
go, neither did it notify about the incomplete install.  In the end, a
restart is enough and required to trigger `install_missing_packages()`
to catch up and install everything.

We have two options here.  (1) to bail out `install_package` with
`return None` and a message to tell the user to restart Sublime, or (2)
to install the packages in a recursive style.

Here we choose (1) which is a simple babystep for a better UX.  (2) is
not possible as all install side-effects need to run serialized.
@kaste
Copy link
Contributor Author

kaste commented May 30, 2025

You might be right. I now see e.g.

                    sublime.set_timeout_async(PackageDisabler.restore_settings, 4000)

and this alone makes it unreliable. (Because the function returns without having its work done completely.) (On the other side, I don't get why this backup_and_restore process is even necessary for installs. E.g. how can a view use a resource from a package that is not even installed? That seems like a noop function for that case.)

The code in disable_packages is otherwise robust and probably ready to be called in a recursive way.

I choose (1) then which is just a babystep for a nicer UX.

Q: When run_install_tasks fails to install all tasks it notifies in the status bar. But why isn't that more in your face? "A restart is required"

@deathaxe
Copy link
Contributor

I don't get why this backup_and_restore process is even necessary for installs.

IIRC, PC just did so forever and I just maintained old behaviors as much as possible. I don't however remember if there were crucial reasons for it.

When run_install_tasks fails to install all tasks it notifies in the status bar. But why isn't that more in your face? "A restart is required"

It primarily depends on who's calling it. Tasks executed via Command Palette by user likely display modal error dialogs. Background tasks, which are automatically started (e.g. all the startup tasks) are designed to not bother users with uncritical failures/errors.

Not saying all choises are perfect. The overall error handling design of PC is not ideal and it is not always easy to judge a failure critical or not on the task or gui layer, as underlying functions just return True, False or None.

I didn't have time to invest, but I carry around an idea of re-designing the whole plugin install/upgrade code to split it into 2 major steps, very much like pip or uv do.

Step 1: Staging, pre-download all requested packages to a temporary directory (in parallel if possible) and resolve dependencies recursively. This should work for both ST packages and python packages alike.

Step 2: install downloaded packages if step 1 succeeded.

It would allow arbritary batching/queuing downloads in step 1 without concerns about partly broken installs if any package fails downloading. Finally if everything required is in-place locally, just extract it - which likely succeeds.

The major differences between installing ST packages and python wheels is just the implementation needed to resolve dependencies.

I however always faild finding a starting point for this with the given architecture. Actually I was more or less focused on finding solutions to replace inefficient API quieries and legacy downloader modules. I have been experimenting with httpx for that purpose, but finally found it suffering from various performance issues and found me requiring to implement various caching and authentication mechanisms to get at least the functionality PC's downloaders currently have.

But your aiohttp approach looks pretty promising alternative to built PC upon.

@kaste
Copy link
Contributor Author

kaste commented Jun 2, 2025

This all makes sense. I tend to give the advise to just "get the ball rolling", to make the code fluid, or to show that the code still can be transformed, by just making simple commits in the right direction but without much overhead planning.

@deathaxe
Copy link
Contributor

As it is not planned to open the door of abusing an existing Package Control.sublime-settings as mechanism to define inter-package dependencies and current support of loading it from any package has a very special use case, this PR probably won't be merged.

@kaste
Copy link
Contributor Author

kaste commented Jun 27, 2025

The PR changed already, it is just about informing the user that something is missing. Is there actually a different plan to support dependencies?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants