COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)
Mozilla Public License 2.0
55 stars 55 forks source link

Test that ddsidl is correct #379

Closed erikbosch closed 2 months ago

erikbosch commented 3 months ago

This PR is related to a discussion in #373.

Previous instructions seemed to be a mashup of installation from source and installation from pypi, see https://github.com/eclipse-cyclonedds/cyclonedds-python

nwesem commented 3 months ago

tested it, looks good to me

nwesem commented 3 months ago

Actually I'm wondering if the output of [poetry run] idlc -l py ./test.idl is supposed to be

No default extensibility provided. For one or more of the aggregated types in the IDL the extensibility is not explicitly set. Currently the default extensibility for these types is 'final', but this may change to 'appendable' in a future release because that is the default in the DDS XTypes specification.
erikbosch commented 3 months ago

Actually I'm wondering if the output of [poetry run] idlc -l py ./test.idl is supposed to be

No default extensibility provided. For one or more of the aggregated types in the IDL the extensibility is not explicitly set. Currently the default extensibility for these types is 'final', but this may change to 'appendable' in a future release because that is the default in the DDS XTypes specification.

Based on what we currently generate I think the warning is correct. I do not know if it can be easily suppressed. We could off course add something like shown in the link below, but as no-one of us seem to use ddsidl we can likely leave it as is.

https://fast-dds.docs.eprosima.com/en/latest/fastddsgen/dataTypes/dataTypes.html#extensibility

sschleemilch commented 2 months ago

Unfortunately this broke poetry install for me. That might be my setup only though (MacOs with Python from Nix)

  - Installing cyclonedds (0.10.5): Failed

  ChefBuildError

  Backend subprocess exited when trying to invoke get_requires_for_build_wheel

  Could not locate cyclonedds. Try to set CYCLONEDDS_HOME or CMAKE_PREFIX_PATH

  at /nix/store/zp80lg9gv25mp11gh4cpwf1lg6y2jkfz-python3.11-poetry-1.8.3/lib/python3.11/site-packages/poetry/installation/chef.py:164 in _prepare
      160│
      161│                 error = ChefBuildError("\n\n".join(message_parts))
      162│
      163│             if error is not None:
    → 164│                 raise error from None
      165│
      166│             return path
      167│
      168│     def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path:

Note: This error originates from the build backend, and is likely not a problem with poetry but with cyclonedds (0.10.5) not supporting PEP 517 builds. You can verify this by running 'pip wheel --no-cache-dir --use-pep517 "cyclonedds (==0.10.5)"'

However, as far as I understood we do not need cyclonedds for our functionality and therefore I would throw it out of our pyproject.toml and update docs to install it manually.

Alternatively we can introduce a group for it so you could do poetry install --with cyclonedds.

erikbosch commented 2 months ago

Possibly some arm64 issue? If necessary we could remove this test "out of pytest" so that you easily could run all other pytests even on a platform where it is is diffiult/impossible to install cyclonedds

nwesem commented 2 months ago

I just remember now that I ran into the same issue until I built cyclonedds lib from source via https://github.com/eclipse-cyclonedds/cyclonedds Did you have to do that too @erikbosch?

Edit: And then point CYCLONEDDS_HOME at the install folder of the build export CYCLONEDDS_HOME=path/to/cyclonedds/install/. I put this in the bashrc that's why i might have missed that

Edit No. 2: I just confirmed this by installing the dev package of cyclonedds, it is needed to install the pip package. For debian this would be apt install cyclonedds-dev

erikbosch commented 2 months ago

Are you running on Mac/Arm64 @nwesem ? I am running on Linux/amd64 and not experiencing any problem. The same is true for CI I think.