-
Notifications
You must be signed in to change notification settings - Fork 48
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
Testing the command "cyclonedds typeof" with a derived topic #242
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have fixed the problem of cyclonedds typeof
on derived types, but I forgot to re-run the CI on this PR after that fix.
It turns out there are still some minor issues ...
For Python ≥ 3.8, it fails because the output of typeof
doesn't match the expectation. The output now correctly includes the definition of Base
and so I think the expectation needs to be adjusted.
On the CI, it fails for Python 3.7 with:
/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/subprocess.py:512: CalledProcessError
----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
File "/home/vsts/.local/bin/cyclonedds", line 5, in <module>
from cyclonedds.tools.cli.main import cli
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/main.py", line 4, in <module>
from .ls import ls
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/ls.py", line 6, in <module>
from .discovery.main import ls_discovery
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/discovery/main.py", line 10, in <module>
from ..idl import IdlType
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/idl.py", line 130
return _type.length, *inner
^
SyntaxError: invalid syntax
Traceback (most recent call last):
File "/home/vsts/.local/bin/cyclonedds", line 5, in <module>
from cyclonedds.tools.cli.main import cli
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/main.py", line 4, in <module>
from .ls import ls
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/ls.py", line 6, in <module>
from .discovery.main import ls_discovery
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/discovery/main.py", line 10, in <module>
from ..idl import IdlType
File "/home/vsts/.local/lib/python3.7/site-packages/cyclonedds/tools/cli/idl.py", line 130
return _type.length, *inner
^
SyntaxError: invalid syntax
----- generated Nunit xml file: /home/vsts/work/1/s/tests/test-output.xml ------
It seems to me that this an issue with the typeof
implementation on Python 3.7. That's not a problem with this PR, but I do wonder if we shouldn't simply retire Python 3.7 support instead of trying to exclude this test in that one case.
You almost certainly have a better idea how relevant Python 3.7 still is. I wouldn't be surprised if it is the standard version in Ubuntu 20.04 or so, but if that's the case, then perhaps we should bump the Ubuntu builds to 22.04 just like we did for the Cyclone core library.
I would be grateful if you would share your view on this.
@dataclass | ||
@annotate.mutable | ||
@annotate.autoid("sequential") | ||
class Base(idl.IdlStruct, typename="Hierarchy.Base"): | ||
fieldA: str | ||
|
||
|
||
DERIVED_IDL = 'module Hierarchy { struct Derived : Base { string fieldB; }; };' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DERIVED_IDL = 'module Hierarchy { struct Derived : Base { string fieldB; }; };' | |
DERIVED_IDL = 'module Hierarchy { @mutable struct Base { string fieldA; }; @mutable struct Derived : Base { string fieldB; }; };' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eboasson! Thanks for fixing the "typeof" command and circling back to this pull request. Your code change suggestion is correct, but I found another minor problem in the _type_checker function. I intend to handle the code changes when I return to the office in three weeks.
Stopping supporting Python 3.7 makes sense, as it has already reached its end-of-life. I have yet to check the Python version on Ubuntu or the end-of-life dates for its versions, but it would make sense to follow what has been done in CycloneDDS core. I would need to think about that a bit further to give you a better answer.
The added test currently fails due to the issue #241.