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

Testing the command "cyclonedds typeof" with a derived topic #242

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adrianomarto
Copy link
Contributor

The added test currently fails due to the issue #241.

Copy link
Contributor

@eboasson eboasson left a 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; }; };'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DERIVED_IDL = 'module Hierarchy { struct Derived : Base { string fieldB; }; };'
DERIVED_IDL = 'module Hierarchy { @mutable struct Base { string fieldA; }; @mutable struct Derived : Base { string fieldB; }; };'

Copy link
Contributor Author

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.

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.

2 participants