Add ., ./lib, ./plugin directories to path for Python plugins
#3110
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: taooceros taooceros has most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Flow.Launcher.Core/Plugin/PythonPluginV2.cs (1)
50-52: Insert paths at the beginning ofsys.pathfor correct module precedenceAppending to
sys.pathplaces your directories at the end, which may cause conflicts if there are modules with the same names elsewhere in the path. Inserting them at the beginning ensures your modules take precedence.Modify the code to insert paths at the beginning:
- sys.path.append(r'{rootDirectory}') - sys.path.append(r'{libDirectory}') - sys.path.append(r'{pluginDirectory}') + sys.path.insert(0, r'{pluginDirectory}') + sys.path.insert(0, r'{libDirectory}') + sys.path.insert(0, r'{rootDirectory}')Flow.Launcher.Core/Plugin/PythonPlugin.cs (3)
1-1: Confirm the necessity of addingusing System;The
using System;directive has been added. Please verify if this is required, as it might be redundant if unused in the code.
71-73: Insert paths at the beginning ofsys.pathfor correct module precedenceAppending directories to
sys.pathmay lead to unintended module resolutions if other installed modules have the same names. Inserting your directories at the beginning ensures that your plugin modules are found first.Modify the code as follows:
- sys.path.append(r'{rootDirectory}') - sys.path.append(r'{libDirectory}') - sys.path.append(r'{pluginDirectory}') + sys.path.insert(0, r'{pluginDirectory}') + sys.path.insert(0, r'{libDirectory}') + sys.path.insert(0, r'{rootDirectory}')
86-89: Reconsider the use of the-Bflag for clarityAlthough you're keeping the
-Bflag to maintain argument positions, it might cause confusion since it's no longer necessary due to thePYTHONDONTWRITEBYTECODEenvironment variable. Consider using an explicit placeholder to maintain argument positions without adding redundant flags.Suggested change:
- _startInfo.ArgumentList.Add("-B"); - _startInfo.ArgumentList.Add(context.CurrentPluginMetadata.ExecuteFilePath); + _startInfo.ArgumentList.Add(""); // Placeholder to maintain argument positions + _startInfo.ArgumentList.Add(context.CurrentPluginMetadata.ExecuteFilePath);Add a comment to explain the placeholder:
// Adding an empty string as a placeholder to keep the argument positions consistent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/PythonPlugin.cs(3 hunks)Flow.Launcher.Core/Plugin/PythonPluginV2.cs(1 hunks)
🔇 Additional comments (4)
Flow.Launcher.Core/Plugin/PythonPluginV2.cs (2)
29-29: Setting PYTHONDONTWRITEBYTECODE to prevent .pyc file generation is appropriate
This change ensures that Python does not generate .pyc files, which can contain location-specific information that hinders portability.
34-63: Ensure proper handling of paths with spaces or special characters
When constructing the code string passed via the -c argument, ensure that paths (rootDirectory, libDirectory, pluginDirectory, filePath) are properly escaped or quoted to handle spaces and special characters. This prevents potential execution errors if plugin directories have such characters.
To verify, consider testing the plugin with directory paths that contain spaces or special characters.
Flow.Launcher.Core/Plugin/PythonPlugin.cs (2)
29-31: Setting PYTHONDONTWRITEBYTECODE to prevent .pyc file generation is appropriate
This change ensures that Python does not generate .pyc files, which can contain location-specific information that hinders portability.
53-95: Ensure proper handling of paths with spaces or special characters
Similar to PythonPluginV2, when passing the code via the -c argument, ensure that all paths are correctly escaped or quoted to handle any spaces or special characters.
Test the plugin with directory paths containing spaces or special characters to ensure it operates correctly.
Yusyuriv
requested review from
Garulf,
JohnTheGr8,
VictoriousRaptor,
jjw24 and
taooceros
|
Thanks for making this, I'll try and take a look this weekend when I have more time. |
|
LGTM. We will want to update how those path are handled in the ci here before we remove them from documentation going forward. |
|
@Garulf do you still want to do more testing? |
|
Would it make sense to also add the win32 package paths to path? Incase they are using the pywin32 package? If they are using the package, this would be needed (so the question is should the plugin dev do it or flow), since pywin32 uses a pth file that doesn't get used with the lib folder |
|
I believe so, thoughts @Yusyuriv ? |
|
I don't think we should add package-specific things to the path because most of the plugin devs won't be using those packages. I added |
|
Sorry for the late reply. I think pywin32 can be very useful for a Python plugin developer. However its unique packaging can cause a lot of friction. I think it would be very helpful to add a path for this package as well. |
|
Hi @Yusyuriv have you got time to add this in or do you want to separate this change out as a todo later issue? |
|
Sure, I'll add it sometime this week. |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: PatternIf the flagged items are 🤯 false positivesIf items relate to a ...
|
|
Done. As far as I know, these are the only two directories that needed to be added:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PythonPlugin.cs (1)
54-86: Consider extracting common Python initialization code.The Python initialization code is duplicated between PythonPlugin and PythonPluginV2. Consider extracting this into a shared helper class.
Create a new helper class:
internal static class PythonPluginHelper { public static string GetPythonInitCode(string rootDirectory, string executeFilePath) { var libDirectory = Path.Combine(rootDirectory, "lib"); var libPyWin32Directory = Path.Combine(libDirectory, "win32"); var libPyWin32LibDirectory = Path.Combine(libPyWin32Directory, "lib"); var pluginDirectory = Path.Combine(rootDirectory, "plugin"); return $""" import sys, os paths = [ r'{rootDirectory}', r'{libDirectory}', r'{libPyWin32LibDirectory}', r'{libPyWin32Directory}', r'{pluginDirectory}' ] for path in paths: if os.path.exists(path): sys.path.append(path) import runpy runpy.run_path(r'{executeFilePath}', None, '__main__') """; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/PythonPlugin.cs(3 hunks)Flow.Launcher.Core/Plugin/PythonPluginV2.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (5)
Flow.Launcher.Core/Plugin/PythonPluginV2.cs (3)
29-29: LGTM! Environment variable addition is a good approach.Using
PYTHONDONTWRITEBYTECODEenvironment variable is a more reliable approach than using the-Bflag, as it ensures consistent behavior across different Python execution methods.
63-67: LGTM! Fallback for non-Python files.The fallback for non-.py files maintains backward compatibility.
34-62: Verify directory existence before adding to sys.path.While the approach to modify sys.path is good, we should verify that these directories exist before adding them to avoid potential issues with non-existent paths.
Consider adding existence checks:
StartInfo.ArgumentList.Add( $""" import sys import os - sys.path.append(r'{rootDirectory}') - sys.path.append(r'{libDirectory}') - sys.path.append(r'{libPyWin32LibDirectory}') - sys.path.append(r'{libPyWin32Directory}') - sys.path.append(r'{pluginDirectory}') + paths = [ + r'{rootDirectory}', + r'{libDirectory}', + r'{libPyWin32LibDirectory}', + r'{libPyWin32Directory}', + r'{pluginDirectory}' + ] + for path in paths: + if os.path.exists(path): + sys.path.append(path) import runpy runpy.run_path(r'{filePath}', None, '__main__') """ );Flow.Launcher.Core/Plugin/PythonPlugin.cs (2)
29-31: LGTM! Well-documented environment variable addition.Good documentation explaining why we prevent .pyc file generation.
90-98: LGTM! Well-documented compatibility handling.Good explanation of why the
-Bflag is retained for compatibility.
The problem
Currently, due to Flow Launcher using embedded Python by default, plugin directory doesn't get added to path. This causes inconvenience for plugin developers. The plugin development documentation for Python suggests using
./libfor installing dependencies and./pluginfor actual plugin code, but developers can't import anything from there because these directories are not in Python's path. They have to add them to path manually in every plugin before they can start importing their code. For example, FlowYouTube plugin:The solution
We can't simply add these paths to
PYTHONPATHenv variable before starting the plugin because we're using embedded Python. It ignores this variable. Flow Launcher users also might be using their own Python installations, so we can't use any solution that would apply only to our embedded Python. So the easiest solution I came up with was instead of launching the files directly (likepython path/to/file.py), run some code that Flow Launcher generates, adding these paths to Python's path, and only then run the actual plugin file viarunpy:Backwards compatibility
I tested this change with several plugins, with them still manually adding directories to path and with them simply importing things. From my tests it seemed like they all continued working normally. Worst case scenario I can see is that either:
Now, to the plugins I tried running. I tested the listed plugins before removing the code marked red in the code blocks and after. They all continued working like before.
v1
GitHub Quick Launcher
No changes here, it's released as a
.pyzfile.FlowYouTube
Search-MDI
ChatGPT
v2
My unreleased plugin I use to test my plugin API v2 library
Notes
While doing this I encountered a problem. v1 plugins expect requests to be sent as the third argument. I couldn't make
-c <code>work after the request because I think it was thinking that the request JSON was file name, so I had to move-c <code>to be the first two arguments. However, Python stops processing arguments after it encounters-c <code>, so I couldn't specify-Bafter it, and I couldn't specify-Bbefore because-c <code>takes up two arguments and the third argument must be the request. So I replaced-Bflag usage with the PYTHONDONTWRITEBYTECODE environment variable that does the same thing.