Plugin system #91

JeanChristopheMorinPerso · 2024-02-03T23:06:22Z

Draft PR for visibility. It kind of works on Linux.

It adds a new plugin system with multiple hooks. There is an example plugin to add support for correctly installing PySide6>=6.3. The plugin takes care of merging all the PySide6 related packages (PySide6, PySide6-Addons and PySide6-Essentials) into one rez package. There is also a shiboken6 plugin that takes care of removing the vendored PySide6 stuff...

$ rez-pip2 PySide6 --prefix /tmp/asd --python 3.11 -- --find-links /tmp/wheels --no-index

$ rez-env PySide6 --paths ~/rez_packages:/tmp/asd -- python -c 'import PySide6,shiboken6; print(PySide6); print(shiboken6)'
<module 'PySide6' from '/tmp/asd/PySide6/6.6.1/9645a50b415bcaad6bcde791aeb0859a43c56501/python/PySide6/__init__.py'>
<module 'shiboken6' from '/tmp/asd/shiboken6/6.6.1/9645a50b415bcaad6bcde791aeb0859a43c56501/python/shiboken6/__init__.py'>

🎉

TODOs:

Add option to disable plugins/hooks
Test if builtin plugins can be overridden
Test on Windows
Write tests
Cleanup the code
Write docs (and automatically generate docs for builtin plugins).

src/rez_pip/rez.py

src/rez_pip/plugins/__init__.py

JeanChristopheMorinPerso · 2024-02-03T23:15:01Z

src/rez_pip/rez.py

@@ -113,8 +136,8 @@ def make_root(variant: rez.packages.Variant, path: str) -> None:
        pkg.pip = {
            "name": dist.name,
            "version": dist.version,
-            "is_pure_python": metadata["is_pure_python"],
-            "wheel_url": wheelURL,
+            "is_pure_python": isPure,


This annoys me. I don't think we really need this...

What is annoying about it?

It's kind of useless and getting the value of "is pure" is annoying. Do you think I should keep it?

src/rez_pip/plugins/PySide6.py

src/rez_pip/cli.py

codecov · 2024-02-04T01:49:50Z

Codecov Report

Attention: Patch coverage is 67.32283% with 83 lines in your changes are missing coverage. Please review.

Project coverage is 77.46%. Comparing base (f3bd30f) to head (361601a).

Files	Patch %	Lines
src/rez_pip/plugins/PySide6.py	50.90%	19 Missing and 8 partials ⚠️
src/rez_pip/cli.py	26.92%	16 Missing and 3 partials ⚠️
src/rez_pip/pip.py	69.69%	9 Missing and 1 partial ⚠️
src/rez_pip/plugins/__init__.py	87.27%	5 Missing and 2 partials ⚠️
src/rez_pip/plugins/shiboken6.py	58.82%	5 Missing and 2 partials ⚠️
src/rez_pip/install.py	50.00%	4 Missing ⚠️
src/rez_pip/rez.py	87.50%	1 Missing and 3 partials ⚠️
src/rez_pip/utils.py	25.00%	2 Missing and 1 partial ⚠️
src/rez_pip/compat.py	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   81.25%   77.46%   -3.79%     
==========================================
  Files           8       12       +4     
  Lines         720      914     +194     
  Branches      150      207      +57     
==========================================
+ Hits          585      708     +123     
- Misses        119      173      +54     
- Partials       16       33      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jean-Christophe Morin <[email protected]>

BryceGattis

Left a couple questions/comments.

BryceGattis · 2024-04-30T12:46:18Z

docs/source/conf.py

    "rez": ("https://rez.readthedocs.io/en/stable/", None),
 }

 # Force usage of :external:
-intersphinx_disabled_reftypes = ["*"]
+# intersphinx_disabled_reftypes = ["*"]


Is this something we are wanting to keep commented out or remove?

I don't remember the exact reason I did this, but I remember that it was causing issues. I'll probably have to investigate this more to remember why I changed that.

BryceGattis · 2024-04-30T12:49:07Z

docs/source/plugins.rst

+=======
+Plugins
+=======
+


Could use a small writeup of what plugins are in rez-pip and how they work at the top here.

Indeed. I'll write something up once I'm fully happy with the plugin system.

BryceGattis · 2024-04-30T13:15:57Z

src/rez_pip/pip.py

+        self.__localPath = path
+
+
+class PackageGroup:


What functionality does this new class provide?

It's mainly a convenience that saves me from having to use zip in multiple places. A package groups contains the grouped packages and also has their distribution objects. It's also slightly easier to work with a list of PackageGroup than a list of list. I first implemented the code without this class, and the result was 🍝.

BryceGattis · 2024-04-30T13:17:05Z

src/rez_pip/plugins/PySide6.py

+
+On Windows, the PySide6/openssl folder has to be added to PATH, see https://inspector.pypi.io/project/pyside6/6.6.1/packages/ec/3d/1da1b88d74cb5318466156bac91f17ad4272c6c83a973e107ad9a9085009/PySide6-6.6.1-cp38-abi3-win_amd64.whl/PySide6/__init__.py#line.81.
+
+So it's at least a 3 steps process:


Note that you only listed 2 steps here 😆

This is outdated I think

BryceGattis · 2024-04-30T13:17:52Z

src/rez_pip/plugins/PySide6.py

+if typing.TYPE_CHECKING:
+    from rez_pip.compat import importlib_metadata
+
+# PySide6 was initiall a single package that had shiboken as a dependency.


Couple of typos in these comments.

BryceGattis · 2024-04-30T13:18:44Z

src/rez_pip/plugins/PySide6.py

+    packages: typing.List[str],
+) -> None:
+    pyside6Seen = False
+    variantsSeens = []


Should this just be variantsSeen?

BryceGattis · 2024-04-30T13:23:29Z

src/rez_pip/rez.py

@@ -113,8 +136,8 @@ def make_root(variant: rez.packages.Variant, path: str) -> None:
        pkg.pip = {
            "name": dist.name,
            "version": dist.version,
-            "is_pure_python": metadata["is_pure_python"],
-            "wheel_url": wheelURL,
+            "is_pure_python": isPure,


What is annoying about it?

JeanChristopheMorinPerso mentioned this pull request Feb 3, 2024

Fail to install PySide6 #64

Open

JeanChristopheMorinPerso commented Feb 3, 2024

View reviewed changes

JeanChristopheMorinPerso force-pushed the plugins branch from 5163f94 to c7edc67 Compare February 4, 2024 00:36

Repository owner deleted a comment from sonarcloud bot Mar 6, 2024

JeanChristopheMorinPerso force-pushed the plugins branch 3 times, most recently from 178c63c to f9b6577 Compare April 28, 2024 23:38

JeanChristopheMorinPerso changed the title ~~Initial commit adding a plugin system and a PySide6 plugin~~ Plugin system Apr 28, 2024

JeanChristopheMorinPerso added 7 commits April 28, 2024 19:58

Initial commit adding a plugin system and a PySide6 plugin

d13b135

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix bug with requirements of merged packages

54d7e18

Signed-off-by: Jean-Christophe Morin <[email protected]>

Simplify registration of builtin plugins

552a169

Signed-off-by: Jean-Christophe Morin <[email protected]>

Add pluggy to project deps

399aa58

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix tests

50ef9fe

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix remaining issues (I think)

2dc8ef5

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix download tests and add a test for skipped downloads

e57ce92

Signed-off-by: Jean-Christophe Morin <[email protected]>

JeanChristopheMorinPerso force-pushed the plugins branch from f9b6577 to e57ce92 Compare April 28, 2024 23:58

JeanChristopheMorinPerso added 4 commits April 28, 2024 21:46

Fix typing

64de12b

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix typing

4b8672f

Signed-off-by: Jean-Christophe Morin <[email protected]>

Add basic test for plugins module

a2a1f9e

Signed-off-by: Jean-Christophe Morin <[email protected]>

Fix typing again

361601a

Signed-off-by: Jean-Christophe Morin <[email protected]>

BryceGattis reviewed Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin system #91

Plugin system #91

JeanChristopheMorinPerso commented Feb 3, 2024 •

edited

Loading

JeanChristopheMorinPerso Feb 3, 2024

BryceGattis Apr 30, 2024

JeanChristopheMorinPerso May 1, 2024

codecov bot commented Feb 4, 2024 •

edited

Loading

BryceGattis left a comment

BryceGattis Apr 30, 2024

JeanChristopheMorinPerso May 1, 2024

BryceGattis Apr 30, 2024

JeanChristopheMorinPerso May 1, 2024

BryceGattis Apr 30, 2024

JeanChristopheMorinPerso May 1, 2024

BryceGattis Apr 30, 2024

JeanChristopheMorinPerso May 1, 2024

BryceGattis Apr 30, 2024

BryceGattis Apr 30, 2024

BryceGattis Apr 30, 2024


		On Windows, the PySide6/openssl folder has to be added to PATH, see https://inspector.pypi.io/project/pyside6/6.6.1/packages/ec/3d/1da1b88d74cb5318466156bac91f17ad4272c6c83a973e107ad9a9085009/PySide6-6.6.1-cp38-abi3-win_amd64.whl/PySide6/__init__.py#line.81.

		So it's at least a 3 steps process:

Plugin system #91

Are you sure you want to change the base?

Plugin system #91

Conversation

JeanChristopheMorinPerso commented Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2024 • edited Loading

Codecov Report

BryceGattis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeanChristopheMorinPerso commented Feb 3, 2024 •

edited

Loading

codecov bot commented Feb 4, 2024 •

edited

Loading