Switch to python 3.8 - Githubissues

rwols commented 4 years ago

Before we can use asyncio in all of its glory, we need to switch to the Python 3.8 runtime. To do that, we have to wait for a few things:

[x] Package Control must implement an upgrade path for existing "dependencies"
[x] The backrefs dependency must be upgraded to 3.8
[x] The dependency bracex must be upgraded to 3.8
[x] The dependency markupsafe must be upgraded to 3.8
[x] The dependency mdpopups must be upgraded to 3.8
[x] The dependency pygments must be upgraded to 3.8
[x] The dependency pymdownx must be upgraded to 3.8
[x] The dependency python-jinja2 must be upgraded to 3.8
[x] The dependency python-markdown must be upgraded to 3.8
[x] The dependency pyyaml must be upgraded to 3.8
[x] The dependency wcmatch must be upgraded to 3.8
[x] sublime_lib must be upgraded to 3.8
[x] lsp_utils must be upgraded to 3.8
[x] have a pull request ready for switching to 3.8 for the LSP package
[x] announce deadline for switching all helper packages

As far as I understand, this is also going to be a hard backwards-incompatible break. So we have to announce it.

rwols commented 3 years ago

Package Control WIP branch is at https://github.com/wbond/package_control/tree/four-point-oh

rchl commented 3 years ago

Realistically also lsp_utils and sublime_lib would have to be updated or most of the LSP-* packages won't work.

I only really use ResourcePath from sublime_lib so I could vendor it but it will probably be updated anyway since it's from an active ST member.

predragnikolic commented 10 months ago

I think the time for Python 3.8 if finally here :)

Just a small note, the list of deps that LSP currently uses is this:

            "bracex",
            "mdpopups",
            "pathlib",
            "wcmatch",

I did a quick experiment.

I have only LSP and LSP-pyright setup. I created a .python-version file at the root of LSP and LSP-pyright. The file just contains 3.8.

Once I did that, I expected that I need to change something in lsp_utils, but I didn't. I restarted ST, both LSP and LSP-pyright were running in 3.8, which is great.

I don't know why I didn't need to change anything in lsp_utils... it looked like PC4 already added lsp_utils to Lib/python38 with the rest of the required LSP dependencies:

~/.config/sublime-text/Lib/python38  ls
bracex                    lsp_utils-3.1.0.dist-info  sublime_lib-1.5.2.dist-info
bracex-2.1.1.dist-info    mdpopups                   wcmatch
coverage                  mdpopups-4.2.2.dist-info   wcmatch-1.2.1.dist-info
coverage-7.4.1.dist-info  package_control.py
lsp_utils                 sublime_lib

jwortmann commented 10 months ago

I don't really know how Package Control works, but I can see two unmerged pull requests https://github.com/wbond/package_control_channel/pull/8713 and https://github.com/wbond/packagecontrol.io/pull/157 for the channel and for the website, whose descriptions indicate that they are required to install dependencies/libraries on the 3.8 environment:

2. opt-in compatible libraries to python 3.8

Why channel_v4.json

Package Control 4.0 handles both 3.0.0 and 4.0.0 schemes well. Just requires 4.0.0 to pull dependencies/libraries for python 3.8.

~~Notably, the pathlib dependency seems to be still restricted to 3.3 in the package_control_channel PR (note that it's also not listed in your ls output):~~

pathlib

So, as far as I understand, the linked PR needs to be merged first, and even after that we can't upgrade to Python 3.8 unless we drop the pathlib dependency. Is that correct?

Edit: Ah, pathlib is of course part of the Python 3.8 stdlib, so we can just remove it from the dependencies and import from the stdlib directly in the code. I guess then it makes sense why the dependency is only for Python 3.3 😄

Also as far as I understand, all LSP-* helper packages will need a .python-version file with 3.8 as well to keep working. So we need to update them all at the same time with a new LSP 2.0.0 (Python 3.8) release. I think this occasion could potentially be used in case we want to do some other breaking API changes. If we want to do something like that, maybe we should start another thread/issue to collect and discuss such possible API changes, before pushing for a 3.8 update. Let me start here with a first example:

Remove the SessionViewProtocol and SessionBufferProtocol classes - as far as I can tell, they don't have any purpose, and in my opinion they make working with parts of the code base very annoying because "Goto definition" for methods from the SessionView and SessionBuffer classes doesn't work (it only opens the empty method stubs from the Protocol classes). Also I have never seen something like this where an interface/protocol is introduced for something that will ever only have a single class which uses the interface. So I'd vote to remove those two Protocol classes.

rchl commented 10 months ago

Remove the SessionViewProtocol and SessionBufferProtocol classes - as far as I can tell, they don't have any purpose, and in my opinion they make working with parts of the code base very annoying because "Goto definition" for methods from the SessionView and SessionBuffer classes doesn't work (it only opens the empty method stubs from the Protocol classes). Also I have never seen something like this where an interface/protocol is introduced for something that will ever only have a single class which uses the interface. So I'd vote to remove those two Protocol classes.

I believe the purpose is to clearly define what is a public interface and what is internal. Especially for the plugin packages accessing those. I'm not sure how well it's working in practice... I suppose there are cases already where the plugins just override the type and access things anyway.

jwortmann commented 10 months ago

I believe the purpose is to clearly define what is a public interface and what is internal. Especially for the plugin packages accessing those. I'm not sure how well it's working in practice... I suppose there are cases already where the plugins just override the type and access things anyway.

Besides the fact that there is no way in Python to enforce this (those are only type annotations in the plugin class methods anyway) and as you wrote plugins can easily workaround it, Python already has a convention to mark private methods with an underscore. So in my opinion those protocol classes have zero advantages and only make working with the code harder if you use tools like Pyright which use the type annotations as source for "Goto Definition". And around half of the methods from the protocol classes are not meant to be used by plugins anyway and instead were just added there to satify the type checker. Furthermore, there are other classes exposed in the plugin API which apparently also work fine without having a protocol class, e.g. Session without a SessionProtocol etc.

I can also see no indication in https://peps.python.org/pep-0544/ that the intention of typing.Protocol would be to hide internal methods, so I still think this is a misuse or at least bad practice here.

rchl commented 10 months ago

I get your point. It seems a bit excessive given that we have only one implementation for each of those protocols.

That said, regardless if misuse or not, it does make things easier for plugin developers to limit the stuff that they can see (type-wise). For example SessionView has a lot of methods that the plugins should not touch (like on_before_remove, on_capability_added_async and other) but at the same time those can't be marked as private as we want to treat those as public within the LSP code base.

I do think removing those would simplify things within the LSP code base which would be nice. But do we want to accept the before-mentioned drawback? Probably the ideal way would be to have more restricted public Session interface that would then return those more restricted interfaces and those would only be used by external plugins, without affecting internal code.

rwols commented 10 months ago

From what I remember, the reason the two protocol classes SessionViewProtocol and SessionBufferProtocol exist is because they avoid a circular import problem. If the code in sessions.py imported one of the two concrete implementation classes then that would result in a circular import. So, the initial reason for them existing was a technical reason.

Maybe that circular import problem doesn't exist anymore, in which case it's totally fine to remove them if possible (from a technical standpoint).

rchl commented 10 months ago

This is most likely still a problem

jwortmann commented 10 months ago

I've checked it and it turns out that both SessionViewProtocol and SessionBufferProtocol are only used in type annotations. Therefore it should be possible to avoid circular imports by using

if TYPE_CHECKING:
    from ..session_view import SessionView
    from ..session_buffer import SessionBuffer

and make those type annotations to be strings. AFAIK the latter is not even necessary anymore on Python 3.8 if you add from __future__ import annotations to the relevant files.

deathaxe commented 9 months ago

FWIW, I am running LSP and all its helpers seamlessly on python 3.8 for years, since it was initially possible to somehow install dependencies for python 3.8 in early PC4.0 dev builds.

Package Control 4 uses https://github.com/packagecontrol/channel to organize python 3.8 dependencies until there's a chance to upgrade packagecontrol.io.

New depednecies/libraries need to be registered at https://github.com/packagecontrol/channel.

So basically any package/plugin can migrate to python 3.8

The only challenge is probably to migrate LSP and all its helpers at the same time as those need to runn all on same plugin_host.

lsp_utils is registered at https://github.com/packagecontrol/channel/blob/8142a3b67d2ee640d8769cbfcec593e6e4df5554/repository.json#L478-L497 to provide smooth transition experience.

predragnikolic commented 9 months ago

So in order for LSP to update, can you help me figure out what is needed:

Add a .python-version with 3.8 in it, to LSP and all LSP-* pacakges.
Remove the pathlib dependecy - it is no longer needed.
Make a release of LSP and LSP-* packages at the same time.
- this is not a issue for LSP- packages in the SublimeLSP org, but we need to figure out what to do for LSP- packages outside of the organization.

Q1 Do I need to add/change something in lsp_utils? Q2 Should I do something else regarding the update process? Q3 Although this is a big change, I don't know if I should see this as a breaking change or not :) how do you see it?

predragnikolic commented 9 months ago

Here is the complete lists of LSP-* packages:

| Package name (maintainer) | Releases(tags) | |---------------------------|----------| | Chialisp (cameroncooper) | >=4000 | | LSP | 3154 - 4069(3154-), >=4070(4070-) | | LSP-angular | 3154 - 3999(st3-), >=4000 | | LSP-astro | >=4070 | | LSP-bash | 3154 - 3999(st3-), >=4000 | | LSP-Bicep | >=4070 | | LSP-biome | >=4070 | | LSP-clangd | >=4070 | | LSP-copilot (TheSecEng) | >=4126 | | LSP-cspell | >=4126 | | LSP-css | 3154 - 4147(st3-), >=4148 | | LSP-Dart | >=4070 | | LSP-Deno | >=4070 | | LSP-dockerfile | 3154 - 3999(st3-), >=4000 | | LSP-elixir | 3154 - 3999(st3-), >=4000 | | LSP-elm | 3154 - 3999(st3-), >=4000 | | LSP-eslint | 3154 - 3999(st3-), >=4000 | | LSP-file-watcher-chokidar | >=4070 | | LSP-flow | >=4070 | | LSP-gnols (jdkato) | >=4070 | | LSP-gopls | >=4070 | | LSP-Grammarly | >=4070 | | LSP-graphql | 3154 - 3999(st3-), >=4000 | | LSP-html | 3154 - 4147(st3-), >=4148 | | LSP-intelephense | 3154 - 3999(st3-), >=4000 | | LSP-jdtls | >=4070 | | LSP-json | 3154 - 4147(st3-), >=4148 | | LSP-julia | >=4095 | | LSP-kotlin | >=4070 | | LSP-lemminx | 3154 - 4069(st3-), >=4070 | | LSP-leo | >=4070 | | LSP-ltex-ls | >=4070 | | LSP-lua | >=4070 | | LSP-marksman | >=4070 | | LSP-metals (scalameta) | 3154 - 3999(st3-), >=4000 | | LSP-OmniSharp | >=4070 | | LSP-PowerShellEditorServices | >=4070 | | LSP-prisma (Sublime-Instincts) | >=4126 | | LSP-promql (prometheus-community) | 3154 - 4069 | | LSP-pylsp | 3154 - 3999(st3-), >=4000 | | LSP-pyright | 3154 - 4147(st3-), >=4148 | | LSP-R | >=4070 | | LSP-rome | >=4070 | | LSP-ruff | 3154 - 3999(st3-), >=4000 | | LSP-rust-analyzer | >=4070 | | LSP-serenata | 3154 - 3999 | | LSP-SourceKit | >=4070 | | LSP-stylelint | 3154 - 3999(st3-), >=4000 | | LSP-svelte | 3154 - 3999(st3-), >=4000 | | LSP-tagml (HuygensING) | 3154 - 4069 | | LSP-tailwindcss | 3154 - 3999(st3-), >=4000 | | LSP-terraform | >=4070 | | LSP-TexLab | >=4070 | | LSP-typescript | 3154 - 3999(st3-), >=4000 | | LSP-vale-ls (errata-ai) | >=4070 | | LSP-VHDL-ls (martinbarez) | >=4070 | | LSP-volar | 3154 - 4147(st3-), >=4148 | | LSP-vue | 3154 - 3999(st3-), >=4000 | | LSP-yaml | 3154 - 4147(st3-), >=4148 | | LSP-anakin | | | LSP-cmake | | | LSP-SonarLint | | | lsp_utils | 3000 - 4069(st3-v), >=4070 |

The following list of LSP- packages are outside of the org. We would need to figure out how to sync the LSP release and the release of LSP- packages outside of the org.

Chialisp (cameroncooper),
LSP-copilot (TheSecEng),
LSP-gnols (jdkato),
LSP-metals (scalameta),
LSP-prisma (Sublime-Instincts),
LSP-vale-ls (errata-ai),
LSP-VHDL-ls (martinbarez)

The following packages will not be updated to py3.8:

LSP-tagml (outside from organization), LSP-serenata, LSP-promql(outside from organization) because they only support ST3.

deathaxe commented 9 months ago

Q1 Do I need to add/change something in lsp_utils?

No. It's working fine as it is.

Q2 Should I do something else regarding the update process?

The 3. steps described, are technically everything, which is needed.

Q3 Although this is a big change, I don't know if I see this a breaking change or not :) how do you see it?

It isn't a breaking change functionality wise.

It is however a breaking change for LSP- helper packages as they need to opt-in to python 3.8 as well to ensure to run on same plugin_host. This probably deserves a milestone or landmark to see which versions of LSP and LSP- are compatible.

That's however only an organizational thing as there's no dependency and version management available accross packages.

At least a message for users would be good, to point this change out.

rchl commented 9 months ago

I don't know if a message to users would be needed if the intention is that there will be no visible changes in the behavior...

I don't think most users would really care about some internal detail regarding the runtime version the package runs on.

rchl commented 9 months ago

Note that when I've tried LSP on python 3.8 before I've noticed that there is some breaking behavior regarding string enum handling. In 3.3 runtime it's not handled any differently than without annotation while in 3.8 I think there were issues with stringifying the value or something...

deathaxe commented 9 months ago

If so, I - as an end user - haven't noticed them.

jwortmann commented 9 months ago

Q3 Although this is a big change, I don't know if I should see this as a breaking change or not :) how do you see it?

I would say this is clearly a breaking change. There could be people using a local helper plugin instead of the "clients" configuration in the settings, because some functionality is only available from the plugin class. And it might not be published to GitHub because they don't want to maintain server installation & updates, or for other reasons. So I would recommend to add a small note about it to the update message (maybe something like "LSP now uses the Python 3.8 plugin host. For plugin compatibility, developers of helper packages need to include a .python-version file into their package now."). And the version number of LSP should be incremented to 2.0.0.

There is (at least) one other package outside of the sublimelsp org that you missed above and which needs to be updated: https://packagecontrol.io/packages/WolframLanguage https://github.com/WolframResearch/Sublime-WolframLanguage/blob/master/plugin.py

predragnikolic commented 9 months ago

I created a ticket to track the progress -> https://github.com/sublimelsp/LSP/issues/2417

I would like to keep the communication in this thread to not clutter #2417 :)

predragnikolic commented 9 months ago

I would propose 9 Mart (Saturday) as the date for releasing py3.8 (three weeks till that).

In order for date to became a reality:

the CI needs to be working - Currently it is not working with 3.8 because UnitTesting is not seeing PC4 dependencies.
nothing unexpected should happen. If the unexpected does happen, we will delay it until the unexpected thing is addressed.

Time will tell, but I would really like py3.8 to become a reality.

predragnikolic commented 9 months ago

I've added the proposed versions for each LSP package and it can be seen in this issue https://github.com/sublimelsp/LSP/issues/2417

The format is the following: - [] PACKGE_NAME CURRENT_TAG -> PROPOSED_TAG

Example: - [] LSP-biome 1.3.2 -> 2.0.0

Others feel free to take a look and suggest changes.

@deathaxe I know that LSP-astro tries to follow the server version releases. I put 3.0.0 from the current 2.7.3, feel free to suggest a change.

rwols commented 9 months ago

I would suggest not picking a "deadline" until UnitTesting works with PC4 dependencies. There is no control over UnitTesting and we can't guarantee it'll even be fixed before March 9.

predragnikolic commented 9 months ago

That 9th March means nothing if the two bullet points get in the way. (currently, one of them is getting in the way)

And I know that things will not be fix by themself, and the reality is probably that the CI will not be fixed by that date.

I want to have an optimistic point of view and if by any chance, the CI gets fixed and nothing unexpected happens, I will be more than ready to proceed with releasing 3.8.

predragnikolic commented 9 months ago

Small update, I do not think that the 9th March will happen. :)

I saw that deathaxe proposed some fix on the LSP discord channel. I haven't took the time to understand it. (I took a look but I didn't understand what to do with that info)

So, I will wait for things to get resolved one at a time, when the CI blocker gets fixed somehow, I will then notify maintainers outside of the organization for the future date.

deathaxe commented 9 months ago

UnitTesting currently ships OS dependend scripts (Bash/Powershell) to roughly perform the following

Sublime Text
Download Package Control
Download/Clone Unittesting
start ST
run "satisfy dependencies" to install coverage
exit ST

then ...

clone package to test
start ST
run Unittest command

... all running on a custom docker image.

What my proposal or better proof of concept does is to perform everything after downloading ST from within the running ST instance using an installer_plugin - which actually already exists to perform step 5.

This way setup and test action could hopefully be merged and ST wouldn't need to be restarted, which might even reduce runtime.

deathaxe commented 9 months ago

P.S.: I wouldn't count on things resolving themself.

predragnikolic commented 9 months ago

P.S.: I wouldn't count on things resolving themself.

I know.. I know 🙂 I just hesitate to open another Pandora box, where I need to invest time to learn.

And thanks for the above explanation.

LDAP commented 8 months ago

When switching to 38 we have to also inform @daveleroy to update the LSP bridge in Debugger.

predragnikolic commented 8 months ago

I have some news! 🎉

LSP and LSP-* packages by SublimeLSP are moving to Python 3.8 on April 19, 2024.

Important Date

Fri Apr 19 2024 17:00:00 GMT+0000

The Plan

All LSP PRs under SublimeLSP will be merged.
And a release will be created for LSP and LSP-* packages under SublimeLSP.

For maintainers of LSP-* packages outside SublimeLSP:

Wait for the LSP release. (I will write an update in this thread once LSP is released)
Merge the PR I've created.
Create a release with a tag.
You're done.

To assist maintainers outside the organization, I've already made the PRs:

Please react with 👀 when you see this message :) cc @cameroncooper, @jfcherng, @TerminalFi, @jdkato, @ayoub-benali, @Ultra-Instinct-05, @martinbarez, @bostick

Other things to know

If any major issues arise before April 19, the release date will be canceled and I'll update this thread accordingly.
LSP-* packages not transitioning to Python 3.8 will stop working.
Sublime Text 3 LSP and LSP-* packages are not affected by this change.

If someone has any questions/concerns feel free to ask.

predragnikolic commented 8 months ago

It makes sense that LSP gets a major version update, But not so much for LSP-* packages.

I planned to do a major version update for LSP-* packages, but after talking to rchl decided to do a minor version update.

I'll update this ticket accordingly soon.

predragnikolic commented 7 months ago

Hello :wave:

in ~one hour the LSP release will be created :)

predragnikolic commented 7 months ago

The LSP v2 release is created -> https://github.com/sublimelsp/LSP/releases/tag/4070-2.0.0

Maintainers who maintain LSP-* packages outside of the SublimeLSP organization. feel free to merge the PR I've created and create a new release.

predragnikolic commented 7 months ago

I think that this can also be closed now :)

daveleroy commented 7 months ago

When switching to 38 we have to also inform @daveleroy to update the LSP bridge in Debugger.

This is complete

sublimelsp / LSP

Switch to python 3.8 #1389

Why channel_v4.json

Important Date

The Plan

Other things to know