Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mavgen_python: Add python3 target with type annotations #666

Merged
merged 4 commits into from
Nov 2, 2022
Merged

mavgen_python: Add python3 target with type annotations #666

merged 4 commits into from
Nov 2, 2022

Conversation

alehed
Copy link
Contributor

@alehed alehed commented Apr 28, 2022

This PR is based on #664 and contains 4 commits. It adds type annotation support in way that should not break mavlink for people stuck on Python2. Everyone else should benefit from the type annotations.

@alehed
Copy link
Contributor Author

alehed commented Apr 29, 2022

Note that the type of file should be

class BinaryWriter(typing.Protocol):
    def write(self, b: bytes) -> int:
        ...

and not Any, but typing.Protocol is a python 3.8 thing and I want to support Python 3.6+ with the type annotations.

@tridge
Copy link
Contributor

tridge commented May 11, 2022

notes from dev call:

  • need to pay special attention to mavproxy (including MAVExplorer), ardupilot autotest suite (Tools/autotest/autotest.py) and mavros
  • need CI (github actions) tests to ensure this works both for py2 and py3
  • need a plan for deployment, so existing scripts automatically get the new generator if running on py3
  • note that most scripts access pymavlink via mavutil

@vooon
Copy link
Contributor

vooon commented May 17, 2022

Mavros doesn't use --lang=Python in it's build, but may use as a weak dependency for console scripts.
I'd suggest leave --lang=Python and decide which implementation to use based on interpreter version. E.g. if major version == 3 => use Python3.

@alehed
Copy link
Contributor Author

alehed commented May 17, 2022

Mavros doesn't use --lang=Python in it's build, but may use as a weak dependency for console scripts. I'd suggest leave --lang=Python and decide which implementation to use based on interpreter version. E.g. if major version == 3 => use Python3.

In this PR if you specify "Python" or nothing (the default is "Python") it will only generate the type annotations if you are running it with python >= python3.6

@tridge
Copy link
Contributor

tridge commented Jun 8, 2022

getting this with py2 build:
python2 setup.py build install --user

  File "build/bdist.linux-x86_64/egg/pymavlink/dialects/v10/ASLUAV.py", line 67
    def __init__(self, buf: Optional[Sequence[int]] = None) -> None:
                          ^
SyntaxError: invalid syntax

byte-compiling build/bdist.linux-x86_64/egg/pymavlink/dialects/v10/minimal.py to minimal.pyc
  File "build/bdist.linux-x86_64/egg/pymavlink/dialects/v10/minimal.py", line 67
    def __init__(self, buf: Optional[Sequence[int]] = None) -> None:
                          ^
SyntaxError: invalid syntax

@alehed
Copy link
Contributor Author

alehed commented Jun 8, 2022

getting this with py2 build:
python2 setup.py build install --user

While these errors are annoying, I don't see anything short of disabling bytecode compiling for those versions that fixes these warnings. Note that like this all files that can be byte-compiled, are byte-compiled.

@alehed
Copy link
Contributor Author

alehed commented Jun 17, 2022

I had to revert the last commit that turned off byte-compilation. It will not create an egg directory if you disable byte compilation, which is what is needed by most places. The syntax errors it shows when building the package using python2 and 3.5 are quite harmless. Also note that we build the package using 3.9, so they won't happen during the official build step.

It seems that python3 support was only tacked on and evolved organically.
This assumes python2.7 and uses consistent types for internal variables
while keeping the externally facing types the same.
This makes sure that somebody using python < 3.6  will
be able to use mavutil like before.
@tridge tridge merged commit 7d5174f into ArduPilot:master Nov 2, 2022
@alehed alehed deleted the add_type_annotations branch November 2, 2022 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants