pyotherside icon indicating copy to clipboard operation
pyotherside copied to clipboard

Fixes PyOtherSideQtRCImporter for submodule imports

Open timegrid opened this issue 1 year ago • 3 comments

It looks like the importer fix for python 3.12 in #131 does not cover imports of submodules. In moment we use importNames() like this:

addImportPath("qrc:/src")
importNames("backend.qml_bridge", ["BRIDGE"], () => { [...] });

This stopped working with the upgrade to python 3.12, #131 did not fix it. When used for a submodule, the path variable is set to the value of __path__ from the parent package according to the reference. By removing this path is None condition, it works again. Not sure though, if this MR will break other usecases though.

@apollo13: could you please test this against your setup?

timegrid avatar May 02 '24 00:05 timegrid

Puh will see about testing that, I will try over the weekend. Can you check what path is in your case? Maybe it would be "more" correct to restore the initial check: https://github.com/thp/pyotherside/pull/131/files#diff-453f8c4ed0f10bda6f41ccfd2ab26d7bb45ecc92355ac956b53c75ff9eb1962aL25

So essentially:

if path is None or all(x.startswith('qrc:') for x in path):

apollo13 avatar May 02 '24 06:05 apollo13

In our application the path parameters for different cases look like this:

backend -> None
backend.qml_bridge -> ['qrc:/src/backend']
backend.models.model_item ->  ['qrc:/src/backend/models']
cffi._pycparser -> ['/media/data/code/moment/venv/lib/python3.12/site-packages/cffi']

So excluding non qrc paths makes sense. Restoring this former check works, too, so I'll adjust this PR. Thanks for the hint.

timegrid avatar May 02 '24 10:05 timegrid

Yeah, that makes sense! Thank for double checking.

apollo13 avatar May 02 '24 10:05 apollo13

@apollo13 @timegrid Is this ready to go in?

thp avatar May 31 '24 06:05 thp

I didn't get around to test it. But since it restores the original check that I dropped and reallows sub imports it is ready imo

On Fri, May 31, 2024, at 08:38, Thomas Perl wrote:

@apollo13 https://github.com/apollo13 @timegrid https://github.com/timegrid Is this ready to go in?

— Reply to this email directly, view it on GitHub https://github.com/thp/pyotherside/pull/134#issuecomment-2141326600, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAT5C7H5IV7U6G2GKHSF3LZFALHTAVCNFSM6AAAAABHCXUEBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBRGMZDMNRQGA. You are receiving this because you were mentioned.Message ID: @.***>

apollo13 avatar May 31 '24 06:05 apollo13

I agree. Additionally some moment users tested a patched version for a while, and everything seems fine with the imports.

timegrid avatar May 31 '24 08:05 timegrid

Any chance, this fix could be released? For moment on archlinux we still need to provide an AUR package instead of the extra package. On other distros with python >= 3.12, it probably needs to be compiled manually until then.

timegrid avatar Jan 17 '25 17:01 timegrid

Any chance, this fix could be released? For moment on archlinux we still need to provide an AUR package instead of the extra package. On other distros with python >= 3.12, it probably needs to be compiled manually until then.

This is now released as 1.6.2

thp avatar Feb 15 '25 14:02 thp