cyclonedds-cxx icon indicating copy to clipboard operation
cyclonedds-cxx copied to clipboard

DomainParticipationFactory?

Open zephod77 opened this issue 3 years ago • 28 comments

Is the DomainParticipationFactory not implemented yet or is it not planned to be? The documentation http://download.prismtech.com/docs/Vortex/apis/ospl/isocpp2/html/a02580.html shows it. Is this the correct documentation for the API?

Also the helloWorld examples were not built automatically as the README says it is.

zephod77 avatar Nov 15 '21 21:11 zephod77

The DomainParticipantFactory is something that exists in the specification and also in what might be called the "traditional" DDS language bindings, but it doesn't exist in the "new" C++ binding.

The traditional language bindings are formed by taking the interface defined in the machine-readable portion of the specification and translating it into $LANGUAGE using the IDL-to-$LANGUAGE mapping. That process yields very unergonomic, CORBA-style language bindings. They do work, but there's not a single language in which it feels right.

Enter the new C++ binding: it provides the same functionality but much more in the modern C++ style. Here, you simply construct a participant directly via the DomainParticipant constructor.

The cyclonedds-cxx repository only implements this new C++ binding. We don't currently have plans to implement the traditional one, it is a pain to use and it seems really silly that people would still voluntarily use it when the new one has been around for almost a decade already. (I do keep a bit of wiggle room, I am not going to say it will never ever be implemented, it all depends ...)

Strictly speaking, what you're looking at is the OpenSplice C++ API documentation, and for Cyclone you should look at cyclonedds.io. Cyclone's documentation is however really unpolished and in practice the information you'll find on this new C++ API for any DDS implementation will match Cyclone's implementation well enough. There'll be some differences, but not that many.

Regarding the examples: the README is in error (or the source is): https://github.com/eclipse-cyclonedds/cyclonedds-cxx/blob/d9234aaf1c00492f37cb06db412f5f68040f8e44/CMakeLists.txt#L188

eboasson avatar Nov 16 '21 08:11 eboasson

Thanks for the response. I have some old (>10yr) code I have to maintain that uses DomainParticipantFactory and it has some issues so I wanted to upgrade. I have always questioned why they used a factory when only one DomainParticipant was created but I can't ask the author. I don't think it should be too difficult to change but old code rots and there is probably all kinds of nasty stuff under the surface. PS. I saw in the roadmap that a Rust language binding is planned. Anything happening with that?

zephod77 avatar Nov 16 '21 15:11 zephod77

In my experience, the vast majority of the application code uses only a small subset of the DDS API, and tends to have it abstracted in a custom framework. With a bit of luck, the old code you're dealing with is in the same category and it might actually be feasible to update it to the new API.

Otherwise we could see if it is feasible to provide enough of the old language binding. I don't think we'll have time to do that in full, but we sure can find some time to see what it would take.

I saw in the roadmap that a Rust language binding is planned. Anything happening with that?

Yes! @sjames has put quite a bit of effort into a Rust binding: https://github.com/sjames/cyclonedds-rs. It isn't part of the Eclipse project but we have tried to help him with (too much) information whenever he had questions and we hope that this can eventually become the first gen official Rust language binding.

eboasson avatar Nov 16 '21 15:11 eboasson

Do you know if there is any documentation for the "Traditional API"? I suspect that you are correct and that I will be able to modify my code to remove calls that are not in the new API but I would need to know the difference between the two.

zephod77 avatar Nov 18 '21 17:11 zephod77

You could pick any of the major DDS implementations, as with the "new" one, the differences are minor. By construction, this API hasn't changed in forever, so the one in the (slightly outdated) OpenSplice on GitHub is one option: https://github.com/ADLINK-IST/opensplice/blob/master/docs/pdf/OpenSplice_refman_CPP.pdf

eboasson avatar Nov 18 '21 18:11 eboasson

I've been trying to install cyclone DDS on my old system. I had to upgrade cmake and bison (BTW is there a way to specify the location of bison? I put it in /usr/local/bin but the build only looked in /usr/bin. Had to make a symbolic link) Now my build is failing because my version of openssl is too old. What is the minimum version of openssl that will be accepted?

zephod77 avatar Nov 23 '21 23:11 zephod77

Hi @zephod77,

Re bison: it looks like you can set BISON_EXECUTABLE to point to the executable: https://stackoverflow.com/questions/40848853/cmake-how-to-use-custom-executable-path-for-bison. I tried running cmake -DBISON_EXECUTABLE=... and it certainly has an effect, but as I didn't go so far as to install bison in some other location, I can't say for sure whether it will actually be enough. (I also suspect this is one of the things one can use CMake "toolchain" files for.)

Re OpenSSL: we support and run our tests with OpenSSL 1.1.1 (currently at 1.1.1l). 3.0.0 seems to work fine but is not used in the CI yet because it currently results in some deprecation warnings for the use of old APIs. We know some embedded systems have difficulties upgrading, so we do a sanity check with 1.0.2u in the CI. We obviously don't support using EOL'd versions of OpenSSL.

Finally, if you don't need DDS Security and/or TLS/TCP support, you can build with ENABLE_SSL=OFF to stop using the OpenSSL library in any way, and ENABLE_SECURITY=OFF to strip out all the security plug-in interfaces in the core code. (They're separate options because, at least in theory, you might want to use Cyclone without having OpenSSL and using your own security plug-ins. I don't expect many want to do that, but we support it 🙂)

eboasson avatar Nov 24 '21 07:11 eboasson

OK! We are getting there. I have cyclonedds built and installed. I used the BISON_EXECUTABLE and SENABLE_SECURITY=OFF (at least for now). Thanks for your help.

Now I am trying to build the C++ binding. I get this error when building:

cyclonedds-cxx/src/idlcxx/src/streamers.c:892:3: error: ‘for’ loop initial declarations are only allowed in C99 mode for (size_t i = 0; i < key->field_name->length; i++) { ^ /cyclonedds-cxx/src/idlcxx/src/streamers.c:892:3: note: use option -std=c99 or -std=gnu99 to compile your code

I tried adding -DCMAKE_CXX_STANDARD=c99 to the configure but that didn't help. Does the code really require -std=c99 or does it think I specified that in the configuration?

zephod77 avatar Nov 24 '21 16:11 zephod77

@zephod77, it is a good thing I waited a bit with responding because my first reaction was wrong — caused by not reading carefully enough!

The cyclonedds-cxx repo consists of two parts, the actual language binding, written in C++, in src/ddscxx; and the IDL compiler backend for generating C++ code, written in C, in src/idlcxx. So this part is supposed to be compiled with a C compiler and I am pretty sure that CMake is doing that. (Why else would it work for others?) That means setting CMAKE_CXX_STANDARD is not going to do the trick, because that applies to C++. There is also a CMAKE_C_STANDARD that applies to C.

Furthermore, the warning fits with using a gcc version that defaults to C89 rather than C99 or even newer revisions, which means you'd be using a pretty old gcc (a quick search tells me 5.4 still used the old standard by default; today it defaults to new ones).

Rather than setting CMAKE_C_STANDARD on the command-line, would you mind adding

set(CMAKE_C_STANDARD 99)

to the src/idlcxx/CMakeLists.txt? And if that works, you might as well do a pull request 😀 (If you do that, you'll have to sign the Eclipse Foundation's contributor agreement for me to be allowed to merge it.)

eboasson avatar Nov 26 '21 06:11 eboasson

Hi @eboasson, it has been a 4 day weekend here so I'm just getting back to this. I tried your suggestion but it had no effect. I got the same error as if it ignored that extra line. When I tried putting the -DCMAKE_C_STANDARD=C99 on the command line, it tells me that it is an unknown option. I did a little research and found that adding "C_STANDARD 99" to the set_target_properties line in that same CMakeLists.txt file helped but now I get a different error:

In file included from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/policy/detail/TCorePolicyImpl.hpp:23:0, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/policy/detail/CorePolicy.hpp:24, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/policy/CorePolicy.hpp:23, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/status/TStatus.hpp:24, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/status/detail/TStatusImpl.hpp:22, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/status/detail/Status.hpp:22, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/core/status/Status.hpp:22, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/topic/detail/Topic.hpp:23, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/topic/Topic.hpp:21, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/include/dds/pub/DataWriter.hpp:23, from /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/src/org/eclipse/cyclonedds/domain/DomainParticipantDelegate.cpp:18: /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/src/org/eclipse/cyclonedds/domain/DomainParticipantDelegate.cpp: In constructor ‘org::eclipse::cyclonedds::domain::DomainParticipantDelegate::DomainParticipantDelegate(uint32_t, const DomainParticipantQos&, dds::domain::DomainParticipantListener*, const dds::core::status::StatusMask&, const ddsi_config&)’: /home/developer/develop/acs_src/KACotsCode/cyclonedds-cxx/src/ddscxx/src/org/eclipse/cyclonedds/domain/DomainParticipantDelegate.cpp:135:84: error: expected ‘)’ before ‘PRIu32’ ISOCPP_THROW_EXCEPTION(ISOCPP_INVALID_ARGUMENT_ERROR, "Invalid domain_id: %" PRIu32, id);

Looks like an extra argument or a missing ','

zephod77 avatar Nov 29 '21 14:11 zephod77

If I remove the PRIu32 from that line it builds. There are lots of warnings and notes but it does compile.

zephod77 avatar Nov 29 '21 14:11 zephod77

PRIu32 is one of many #defines in inttypes.h that define the formatting codes needed for the various (u)intN_t fixed-size types that are defined in stdint.h. That's been around since C99, and should have been included, and looking at the C++ binding, I think it is assumed that these PRI... macros are pulled in by including stdint.h from cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp, but that isn't necessarily the case.

As a quick experiment, I'd try adding inttypes.h in cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp.

eboasson avatar Nov 29 '21 15:11 eboasson

I dug through the includes ... I suspect the inttypes.h included by the C++ compiler doesn't define it because (indirectly) DomainParticipantDelegate.cpp includes dds/ddsrt/types.h and then dds/ddsrt/types/posix.h, which includes inttypes.h.

The weird thing here is that Cyclone core repository heavily relies on it and that therefore these definitions must be available. So it isn't like Solaris with a pre-C99 inttypes.h not defining all of them (literally a quarter of a century ago) or gcc after the mid-90s patching the system includes (by a script named "fixincludes") to patch things up and failing in this one case.

If you can't find out why it doesn't include them from C++ while does include them from C, the definitions you need are easy enough, for example PRIu32 is defined as "u" on all common platforms; PRIx32 as "x"; PRId64 typically as either "lu" or "llu" (though for Visual Studio it was "I64" a decade or two back, IIRC). But it really sounds more like some confused system include path.

eboasson avatar Nov 29 '21 15:11 eboasson

I trying to install this on a CentOS 7 system. On this system the /usr/include/inttypes.h file contains this:

/* The ISO C99 standard specifies that these macros must only be defined if explicitly requested. */ #if !defined __cplusplus || defined __STDC_FORMAT_MACROS

before all the PRIxxx defines and __cplusplus is defined so therefore no PRIxxx defines. Interestingly on my Ubuntu 20.04 system, this line is gone.

cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp simply contains an include of <stdint.h> which won't help. Any sugestions on the best way to overcome this? Maybe add the PRIxxx defines in that cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp file?

zephod77 avatar Nov 29 '21 17:11 zephod77

Interesting — and our CI builds on Ubuntu so doesn't run into this problem!

It looks like other people have run into the same problem (albeit with different projects): https://github.com/WAVM/WAVM/pull/266 and there:

#if defined(__GNUC__) && __GNUC__ < 5
#define __STDC_FORMAT_MACROS
#endif

is suggested.

If that works, I can live with adding that to cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp

eboasson avatar Nov 29 '21 17:11 eboasson

OK. If I add that #if clause to cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp and change the #include <stdint.h> to #include <inttypes.h>, then it will build.

/usr/include/inttypes.h includes <stdint.h> on both the Ubuntu system and the CentOS7 system.

zephod77 avatar Nov 29 '21 18:11 zephod77

BTW, is there a reason that the maximum domain id is hard-coded to 230?

src/ddscxx/src/org/eclipse/cyclonedds/domain/DomainParticipantDelegate.cpp, line 134:

if ((id != org::eclipse::cyclonedds::domain::default_id()) && (id > 230)) { ISOCPP_THROW_EXCEPTION(ISOCPP_INVALID_ARGUMENT_ERROR, "Invalid domain_id: %" PRIu32, id);

domain_id in DomainParticipantDelegate is defined as a uint32_t.

zephod77 avatar Nov 29 '21 19:11 zephod77

Hi @zephod77, I believe the maximum domain id of 230 is set by the DDS specification though I can't find an official reference at this time.

k0ekk0ek avatar Nov 29 '21 21:11 k0ekk0ek

@zephod77 the origin of the 230 is that the DDSI spec has a default port mapping for UDP that uses 7400+250*(domain id)+10+2*(participant index)+1 as the highest port number. I don't know why it came out as 230 instead of 232 though. The spec also requires that you can change the mapping, and that means the 230 is really not relevant, the real requirements are on the port mappings.

In Cyclone's core, all domain ids < 2^32-1 are valid. This 230 is some historical artefact of a decision in OpenSplice to limit domain ids. It should be removed here.

eboasson avatar Nov 30 '21 10:11 eboasson

OK so I will create an account and create a pull request will the changes to src/idlcxx/CMakeLists.txt, cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp and the deletion of the check of domain id in src/ddscxx/src/org/eclipse/cyclonedds/domain/DomainParticipantDelegate.cpp.

I am assuming that you are OK having all three changes in one pull request?

zephod77 avatar Nov 30 '21 13:11 zephod77

OK so I will create an account and create a pull request will the changes

Thanks!

I am assuming that you are OK having all three changes in one pull request?

Yes, that works for me.

eboasson avatar Nov 30 '21 15:11 eboasson

Hmm... Not so sure about the changes to cxx/src/ddscxx/include/dds/core/detail/inttypes.hpp now. A comment in that files says:

Under toolchains that support inttypes.h...

which to me suggests that inttypes.h is not always available on all systems so changing #include<stdint.h> to #include <inttypes.h> would break those systems.

Also when I use a large number for the domain_id, I get an error when creating a DomainParticipant:

1638289306.879414 [2147483647] cyclonedds: Invalid port mapping: port number(s) of out range: multicast discovery 536870919150, multicast data 536870919151

Presumably, this has to do with the calculations of the default ports that @eboasson mentioned earlier. Can you point me to the documentation of the config file so I can account for this?

zephod77 avatar Nov 30 '21 16:11 zephod77

Unable to push my changes. I created a personal access token but I still get a 403 error.

zephod77 avatar Dec 01 '21 14:12 zephod77

@zephod77, I don't see a cyclonedds-cxx repository on you profile. Are you perhaps trying to push to this repository? If so, nobody is allowed to push directly except for committers. Best way to go about this sort of thing is to fork the repository, push to your personal fork and issue a pull request from there.

k0ekk0ek avatar Dec 01 '21 14:12 k0ekk0ek

I tried forking the repo and then on my local machine I entered

$ git remote set-url origin https://github.com/zephod77/cyclonedds-cxx.git

then tried to push the commit I had done on the clone of the main repo. I entered my PAT when it asked for a password but now it thinks that was a password and not a PAT. Do I need a different PAT for the forked repo?

zephod77 avatar Dec 01 '21 15:12 zephod77

I personally use SSH instead of HTTPS, but I think this answers your question(?)

k0ekk0ek avatar Dec 01 '21 15:12 k0ekk0ek

Yes, Thanks. That worked as far as pushing my change and creating a pull request but now I have some other issues. I was not able to run the test suite because GoogleTest requires GCC 5 or greater and my system has GCC 4.8.5 and some of the tests are failing. Also, the e-mail I use for Eclipse is different to the one I use for github so I may have to create another so I can sign the agreement.

zephod77 avatar Dec 01 '21 16:12 zephod77

Also when I use a large number for the domain_id, I get an error when creating a DomainParticipant:

1638289306.879414 [2147483647] cyclonedds: Invalid port mapping: port number(s) of out range: multicast discovery 536870919150, multicast data 536870919151

Presumably, this has to do with the calculations of the default ports that @eboasson mentioned earlier. Can you point me to the documentation of the config file so I can account for this?

https://github.com/eclipse-cyclonedds/cyclonedds/blob/master/docs/manual/options.md#cycloneddsdomaindiscoveryports

eboasson avatar Dec 02 '21 13:12 eboasson