Skip to content

Conversation

@jjw24
Copy link
Member

@jjw24 jjw24 commented Jan 31, 2021

If any Python plugins exist upon start up and the PythonDirectory in Settings is empty:

  1. Check env "PATH" if Python path exists and set automatically, else
  2. Prompt the user if they would like to install or select a Python directory,
  3. If chosen to install, use Droplex library to download and install Python 3.9, then set the required variables for the plugin,
  4. If chosen to select the directory, check location exists then set the required variables.
  5. Load Python plugins

@jjw24 jjw24 added the enhancement New feature or request label Jan 31, 2021
@jjw24 jjw24 self-assigned this Jan 31, 2021
@jjw24 jjw24 linked an issue Jan 31, 2021 that may be closed by this pull request
@Zeroto521
Copy link
Member

Zeroto521 commented Jan 31, 2021

I want to know which python version will be installed. Complete version contains interpreter, idle and some script(includes pip and others).
So the installed python with interpreter and scripts will be fine

@jjw24
Copy link
Member Author

jjw24 commented Jan 31, 2021

Python 3.9 will be installed

else
{
var installation = Task.Run(async () => { await DroplexPackage.Drop(App.python3_9_1).ConfigureAwait(false); });
installation.Wait();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emmm since we are waiting it, why do we need to wrap it within the Task.Run?

Copy link
Member Author

@jjw24 jjw24 Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taooceros this should be synchronous, is there another way to do this without converting the calling methods async?

Copy link
Member

@taooceros taooceros Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply Wait the task do be done. DroplexPackage.Drop(App.python3_9_1).Wait(timeout), while timeout is the time to be waited. However I don't think would be great to let user wait downloading python at initialization, since it takes a while and will block other Flow's functionality.
I think this should be done async, disabling python plugin first if python not installed (also ask whether user has installed python similar to what has done in Everything plugin), and once it has downloaded successfully, enabling them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine because the installation has a prompt from Python, so user knows it's installing.

It does also ask the user if they want to select the location beforehand as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok I haven't looked at the newer part of the code.
However, I still think we should wait python installation at initialization, but let it does the job on background.
Is it possible to disable all python plugin and then later enable them after python installed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think python includes a great cdn so users in China may encounter significant of time of downloading it. However, I do agree with you that disabling and enabling plugins will bring quite a mess with at this stage.
Maybe don't load python plugin, and force restarting Flow after Python has installed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python install file is only 28mbs, can you test it out see if it takes long

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python install file is only 28mbs, can you test it out see if it takes long

Will take a try.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm seems that people in China cannot directly finish download python because of timeout. I have encountered that the task was cancelled when I try to do this.
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for testing. How long before it's timeout?

Is it timing out on connection or during the downloading?

@taooceros taooceros added this to the 1.7.0 milestone Feb 1, 2021
@pc223
Copy link
Contributor

pc223 commented Feb 2, 2021

Just quick test this, when python 3.9 missing, Flow will auto install python 3.9 👍

Though my plugin is not usable right now (previously works with Flow-1.6.0), got 2 problems, related, but maybe outside scope of this PR:

  1. When python packages missing (like flowlauncher, python-dotenv,...), Flow doesn't make a warning, should be at least an alert msgbox.
  2. An exception related to RPC/Json stuff
2021-02-02 22:35:16.5874|ERROR|JsonRPCPlugin.Query|-------------------------- Begin exception --------------------------
2021-02-02 22:35:16.6100|ERROR|JsonRPCPlugin.Query|Exception when query <ts>
2021-02-02 22:35:16.6272|ERROR|JsonRPCPlugin.Query|Exception full name:
 <System.InvalidOperationException>
2021-02-02 22:35:16.6429|ERROR|JsonRPCPlugin.Query|Exception message:
 <The JSON property name for 'Flow.Launcher.Core.Plugin.JsonRPCQueryResponseModel.Result' collides with another property.>
2021-02-02 22:35:16.6625|ERROR|JsonRPCPlugin.Query|Exception stack trace:
 <   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(JsonClassInfo jsonClassInfo, JsonPropertyInfo jsonPropertyInfo)
   at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
   at System.Text.Json.JsonSerializer.Deserialize(String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Flow.Launcher.Core.Plugin.JsonRPCPlugin.DeserializedResult(String output) in C:\projects\flow-launcher\Flow.Launcher.Core\Plugin\JsonRPCPlugin.cs:line 66
   at Flow.Launcher.Core.Plugin.JsonRPCPlugin.Query(Query query) in C:\projects\flow-launcher\Flow.Launcher.Core\Plugin\JsonRPCPlugin.cs:line 39>
2021-02-02 22:35:16.6863|ERROR|JsonRPCPlugin.Query|Exception source:
 <System.Text.Json>
2021-02-02 22:35:16.7061|ERROR|JsonRPCPlugin.Query|Exception target site:
 <Void ThrowInvalidOperationException_SerializerPropertyNameConflict(System.Text.Json.JsonClassInfo, System.Text.Json.JsonPropertyInfo)>
2021-02-02 22:35:16.7229|ERROR|JsonRPCPlugin.Query|Exception HResult:
 <-2146233079>
2021-02-02 22:35:16.7402|ERROR|JsonRPCPlugin.Query|-------------------------- End exception --------------------------

@pc223
Copy link
Contributor

pc223 commented Feb 2, 2021

Right now, Flow is using Python 3.9 directly, without virtualenv, this can be pretty annoying for python 3.9 user that use it for something else other than Flow.

So, after Flow install Python 3.9, I think Flow should make a new virtualenv, python -m venv flow_python_env, (takes like 20 seconds) that means Flow can freely install/uninstall any packages it want, no fear of conflict,

and change the python path to something like this in the settings page: flow_python_env\scripts\python.exe instead of the current one.

@jjw24 jjw24 modified the milestones: 1.7.0, 1.8.0 Feb 3, 2021
@jjw24
Copy link
Member Author

jjw24 commented Feb 4, 2021

lets tackle this separately. This PR is to get python installed. We need to figure out how to auto install/uninstall requirement.txt cleanly

@taooceros
Copy link
Member

@pc223 the rpc error has been fixed in #324
You can take a try on the build version of dev branch.
https://ci.appveyor.com/api/buildjobs/k827q6qyfge1a9l0/artifacts/Output%2FPackages%2FFlow-Launcher-v1.7.0.exe

@pc223
Copy link
Contributor

pc223 commented Feb 5, 2021

@pc223 the rpc error has been fixed in #324
You can take a try on the build version of dev branch.
https://ci.appveyor.com/api/buildjobs/k827q6qyfge1a9l0/artifacts/Output%2FPackages%2FFlow-Launcher-v1.7.0.exe

Nice! I'll try it out when I got the time 😄

@taooceros
Copy link
Member

taooceros commented Feb 7, 2021 via email

@Zeroto521
Copy link
Member

Python 3.9 will be installed

This version will install the idle (GUI). The general user doesn't need that thing.

image

And the main version is 3.8 could be better because 3.9 released recently.

I recommend installing miniconda

Miniconda only contains interpreter and scripts.

@Zeroto521
Copy link
Member

Zeroto521 commented Feb 9, 2021 via email

@Zeroto521
Copy link
Member

Anyway package is the problem of next step not now.

@jjw24
Copy link
Member Author

jjw24 commented Feb 10, 2021

This build from 20f895c should resolve download timeout issues. Thanks @taooceros

@jjw24 jjw24 requested a review from taooceros February 11, 2021 03:01
@jjw24
Copy link
Member Author

jjw24 commented Feb 11, 2021

This is for 1.8.0 release, do not merge if approved.

@taooceros
Copy link
Member

taooceros commented Feb 11, 2021

Are there any thing we still need to do for 1.7.1?

@jjw24
Copy link
Member Author

jjw24 commented Feb 11, 2021

Hmm not at this stage. Although we don't have anything pending to go out urgently in 1.8.0 as well right, figure we might catch more bugs for 1.7.1

@taooceros
Copy link
Member

Hmm not at this stage. Although we don't have anything pending to go out urgently in 1.8.0 as well right, figure we might catch more bugs for 1.7.1

I think we should at least release the fix in #322 since it blocks python plugins from working.

@jjw24
Copy link
Member Author

jjw24 commented Feb 11, 2021

Hmm not at this stage. Although we don't have anything pending to go out urgently in 1.8.0 as well right, figure we might catch more bugs for 1.7.1

I think we should at least release the fix in #322 since it blocks python plugins from working.

Yeah makes sense, let's release 1.7.1, let's still hold off on 1.8.0 PRs for a bit to see if anymore bugs come up. What do you think?

@taooceros
Copy link
Member

Yeah agree with you. Actually, I think we can release a pre-release before any large change, and see whether some bugs can be catched by that so that we can fix it before official release.

}
else
{
Log.Error("PluginsLoader",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if user choose to install python on other path instead of the default path? I think we should allow selection if that it not found.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to auto set path after auto install. Users can choose the path before hand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I mean what if user don't choose to install python on the default path?

Copy link
Member Author

@jjw24 jjw24 Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy Chinese New Year @taooceros :)

The installation will just install to the default location, it's a passive install, no prompt for install location

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy New Year!!! 😃😃
Oh I see. That will be great!

@jjw24
Copy link
Member Author

jjw24 commented Feb 11, 2021

Yeah agree with you. Actually, I think we can release a pre-release before any large change, and see whether some bugs can be catched by that so that we can fix it before official release.

Well, I don't know if people download that though, right now pre-releases needed to be manually downloaded

@taooceros
Copy link
Member

I think wox allows user to choose downloading prerelease. Isn't flow containing one?

@jjw24
Copy link
Member Author

jjw24 commented Feb 11, 2021

I think wox allows user to choose downloading prerelease. Isn't flow containing one?

Nah we haven't built it in. I would prefer something like squirrel's diff release where update is on new changes rather than the whole app. Problem with pre-releases is that it will just sit there forever and too much reliance on it to catch bugs is not effective, nobody wants to use a buggy version.

@taooceros
Copy link
Member

emm you are right, but we still need more testing before large release, because sometimes it is hard for us to find bugs during our personal usage. Maybe we can ask someone who are willing to try the new feature to take a try on the prerelease, and after it becomes stable, then we release the official release.
I think the squirrel diff should be addressed later, and it is a feature of squirrel, though I don't understand why it doesn't generate the diff file for now. It should be quite useful for Flow because flow is self-contained, using diff update can avoid duplicate downloading the same .net core runtime.

@jjw24
Copy link
Member Author

jjw24 commented Feb 12, 2021

@taooceros This is good to go out for 1.8.0?

@taooceros
Copy link
Member

@taooceros This is good to go out for 1.8.0?

Ah yes!

@jjw24
Copy link
Member Author

jjw24 commented Feb 14, 2021

Python 3.9 will be installed

This version will install the idle (GUI). The general user doesn't need that thing.

image

And the main version is 3.8 could be better because 3.9 released recently.

I recommend installing miniconda

Miniconda only contains interpreter and scripts.

Good pick up, I will need to look into miniconda along with the other suggestions and will change it later if needed.

@jjw24
Copy link
Member Author

jjw24 commented Feb 14, 2021

We can discuss this further in the discord plugin-dev channel. Once decided it's just a matter of changing the droplex package.

@jjw24 jjw24 merged commit 040a590 into dev Feb 14, 2021
@jjw24 jjw24 deleted the add_python_installation branch February 14, 2021 01:24
@jjw24 jjw24 mentioned this pull request Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Download dependent apps/services/tools automatically

5 participants