Provide Pyro5 compatibility to the remote door utility

[pevogam]

The main adaptation that was needed comes down to dropping the previous expose-free approach and dynamically exposing each relevant function and class in addition to the previous autoproxy steps.

To allow for modules and classes that the shared remote object has not explicitly imported or are not detectable as needing exposing, the previous whitelist argument has been extended to allow exposing predefined classes and serializing predefined exceptions that could be used and raised during remote use.

[pevogam]

There are many modules out there that have Too many positional arguments and I believe it should be just a recommendation. I cannot see the isolation tests here in further details but they all pass locally.

[ldoktor]

Hello @pevogam, thanks for keeping the remote door up and running. My only hard requirement is the virttest.utils_params removal as this is supposed to be generic project. I hope you'll find a better place for this tweak inside avocado-vt (could be a post-init step or whatever...).

As for the rest, I'd prefer having it split into multiple incremental changes, but since this is "an extension" and not core aexpect, it's not a strong requirement. It'd just be easier to follow as, if I got it right, some changes are not really related to the pyro change but are rather improvements or refinements.

[pevogam]

Hi @ldoktor and thanks so much for spending the effort to review this, I know such resources are precious.

Hello @pevogam, thanks for keeping the remote door up and running. My only hard requirement is the virttest.utils_params removal as this is supposed to be generic project. I hope you'll find a better place for this tweak inside avocado-vt (could be a post-init step or whatever...).

I have pushed changes regarding the extra hard-coding point and typos you mentioned.

As for the rest, I'd prefer having it split into multiple incremental changes, but since this is "an extension" and not core aexpect, it's not a strong requirement. It'd just be easier to follow as, if I got it right, some changes are not really related to the pyro change but are rather improvements or refinements.

The refinements are all touching upon the same changes and we cannot make the remote door compatible with Pyro5 without them (or the tests and usage would break at any further intermediary steps) as the only significant refining I can think of is extending the local object sharing with additional exposing flexibility, all of which is a requirement for using Pyro5 in any way.

Regarding the CI, should we disable the Too many positional arguments globally since so many modules have this problem?

[pevogam]

I will run some more integration tests during the weekend, perhaps I will separate the serialization part is the second major part that wasn't needed for Pyro5 - you are right.

[ldoktor]

Thanks, this version looks better as for me it's mostly (see comments in-line) ready to be merged once it passes the CI (I haven't tested it functionally, just reviewed it). As for the too-many pylint failures, simple add them to the pylint: disable on line 56. (still if you prefer to split it into multiple commits, feel free to do so, but as it's not core I don't object if you keep it as is...)

[pevogam]

Thanks a lot @ldoktor, I will take a look for a second and hopefully final round of updates (from your feedback and some of our own integration testing) by some time tomorrow and then all of this will be finalized for you.

[pevogam]

Alright, I pushed some separation but I will likely need some further guidance regarding the CI here. What I said above is that it shows the R0917 not just for the remote door module but a multitude of other modules we haven't touched here. I compared with a passing CI on the main branch and none of these errors are displayed. It could be that they are only displayed if other problems are found which would make pylint or pycodestyle checks much harder to debug to begin with (they should not show an error if it is really disabled by the user) but I might be wrong. They don't show any of the previously disabled errors when the CI passes so I cannot at least diff against it to discover which errors are the new ones specific to the pull request.

Then in addition, I have a pycodestyle error with the little imitating class that I want to disable via noqa but this doesn't seem to be respected by the current linter script used by the project. Perhaps it is not possible and I should simply abide by the somewhat inapplicable requested empty line there?

[ldoktor]

The R0917 is related to the new (well 3 months old) pylint update so let's disable it here #137

As for the noqa this tag works for lgtm and CodeQL but pylint requires to explicitly list the errors by pylint: disable=XXX. If you do it on a separate line it'll disable it for the whole file (or until you re-enable it) but it's better to just put it on the line where you intend it to work (then it'll be only disabled for that line)

[pevogam]

The R0917 is related to the new (well 3 months old) pylint update so let's disable it here #137

Looks good! So it was a catch overall.

As for the noqa this tag works for lgtm and CodeQL but pylint requires to explicitly list the errors by pylint: disable=XXX. If you do it on a separate line it'll disable it for the whole file (or until you re-enable it) but it's better to just put it on the line where you intend it to work (then it'll be only disabled for that line)

The problem is that # pylint: disable=E301 which is what I originally tried ends up in

aexpect/remote_door.py:84:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'E301' (unknown-option-value)

locally and in the ignore comment being, well, ignored by pylint in the CI. So I assumed that the custom linter here internally invokes pycodestyle.

[ldoktor]

The R0917 is related to the new (well 3 months old) pylint update so let's disable it here #137

Looks good! So it was a catch overall.

Just a new check in pylint. It'd be nicer to adjust the code to cope with those but there is not much time in doing this right now.

As for the noqa this tag works for lgtm and CodeQL but pylint requires to explicitly list the errors by pylint: disable=XXX. If you do it on a separate line it'll disable it for the whole file (or until you re-enable it) but it's better to just put it on the line where you intend it to work (then it'll be only disabled for that line)

The problem is that # pylint: disable=E301 which is what I originally tried ends up in

aexpect/remote_door.py:84:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'E301' (unknown-option-value)

locally and in the ignore comment being, well, ignored by pylint in the CI. So I assumed that the custom linter here internally invokes pycodestyle.

It's likely you have an older pylint, github uses 3.3.1. You can upgrade it by pip install -U pylint --user.

[pevogam]

As for the noqa this tag works for lgtm and CodeQL but pylint requires to explicitly list the errors by pylint: disable=XXX. If you do it on a separate line it'll disable it for the whole file (or until you re-enable it) but it's better to just put it on the line where you intend it to work (then it'll be only disabled for that line)

The problem is that # pylint: disable=E301 which is what I originally tried ends up in

aexpect/remote_door.py:84:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'E301' (unknown-option-value)

locally and in the ignore comment being, well, ignored by pylint in the CI. So I assumed that the custom linter here internally invokes pycodestyle.

It's likely you have an older pylint, github uses 3.3.1. You can upgrade it by pip install -U pylint --user.

Maybe I miss something from what you originally meant but you can see that even the error ID is not compatible with the numbering done my pylint. The error also happens despite pylint giving its 10/10 grade. So it is strictly PEP8 related. However, I give up trying to convince the inspektor or frontend that noqa should have worked there and simply added the extra empty line. It might be something to bring up to the inspektor project but best to move on here -> CI is passing now.

[pevogam]

I fixed another typo with an extra push. Considering aexpect has last been released more than a year ago what are your thoughts on another tag+release? Perhaps @lmr is the person to turn to regarding feedback about this?

[ldoktor]

Thanks, looks good to me, let me ping Lucas WRT release.