gregdavill / KiBuzzard

MIT License
425 stars 33 forks source link

Sys.path gets bloated with kibuzzard includes #75

Closed qu1ck closed 2 years ago

qu1ck commented 2 years ago

KiCad reloads plugins every time pcbnew reopens and on plugin refresh action.

The way kibuzzard currently inserts it's own dependencies in system path leads to entries being duplicated on every plugin reload.

  1. There is no need to modify system path, just do relative imports .deps.fonttools etc. This has the benefit of ensuring your packaged version of the lib is used in your code and not something else that some other plugin may have put in sys path before your entry.
  2. If you insist on modifying sys path at least check if the entry you are adding is already there.
qu1ck commented 2 years ago

Btw kicad's own paths also get duplicated and I'll report that on gitlab or fix it myself soon.

gregdavill commented 2 years ago

I didn't think that worked with the current directory structure. Since deps ends up being one level up from buzzard So you have to do ..deps.fonttools.Lib.fontTools.ttLib. Which returns an error.

But I'll take another look.

gregdavill commented 2 years ago

A bit deeper into this now. The main reason I found that relative imports didn't work in this case, was that fonttools itself doesn't use relative imports.

So even if KiBuzzard uses something like this:

from ..deps.fonttools.Lib import fontTools

from fontTools.ttLib import ttFont
from fontTools.pens.recordingPen import RecordingPen
from fontTools.pens.basePen import decomposeQuadraticSegment

I still get an error from fonttools itself, since it's not doing a relative import. Appending to the path is the easiest solution.

Traceback (most recent call last):
  File "/usr/lib/python3.9/runpy.py", line 188, in _run_module_as_main
    mod_name, mod_spec, code = _get_module_details(mod_name, _Error)
  File "/usr/lib/python3.9/runpy.py", line 147, in _get_module_details
    return _get_module_details(pkg_main_name, error)
  File "/usr/lib/python3.9/runpy.py", line 111, in _get_module_details
    __import__(pkg_name)
  File "/home/greg/Projects/KiBuzzard/KiBuzzard/__init__.py", line 48, in <module>
    from .plugin import KiBuzzardPlugin
  File "/home/greg/Projects/KiBuzzard/KiBuzzard/plugin.py", line 15, in <module>
    from .buzzard.buzzard import Buzzard
  File "/home/greg/Projects/KiBuzzard/KiBuzzard/buzzard/buzzard.py", line 5, in <module>
    from ..deps.fonttools.Lib import fontTools as fontTools
  File "/home/greg/Projects/KiBuzzard/KiBuzzard/deps/fonttools/Lib/fontTools/__init__.py", line 4, in <module>
    from fontTools.misc.py23 import *
ModuleNotFoundError: No module named 'fontTools'
gregdavill commented 2 years ago

Here is a fix in place to avoid adding duplicates.

I do realise that using paths like this is not perfect. I'll take another look at this later.

qu1ck commented 2 years ago

Yeah if libs you use have absolute imports and are not easy to patch then modifying system path is the only way. It is a brittle solution, expect things to conflict eventually when other plugins start doing the same.

gregdavill commented 2 years ago

Is this a better solution? I'm limiting the scope of the path change to only cover the importing of modules.

qu1ck commented 2 years ago

It's a good improvement. It doesn't guarantee that you will load your version of the deps. If a module is already imported it will be used without reload even if paths changed. But at least this doesn't pollute global variables.