cyclonedds Rename header directory dds -> cyclonedds

Fix #249 Code pointing to the old headers should still compile, but it will show an advisory message when built, suggesting to update #include paths.

Oct 19 '19 22:10 rotu

@eboasson, maybe instead of "cyclonedds" use a shorter form?

For example "#include "cyclonedds/ddsrt/log.h"" -> #include "cydds/ddsrt/log.h"".

Oct 23 '19 19:10 i-and

While better than "dds", I don't think "cydds" is as good an identifier. I want the top-level include folder to make it obvious what these files are from.

Oct 23 '19 19:10 rotu

From the point of view of the program code the prefix length exceeds the length of all subsequent components:

#include "cyclonedds/ddsi/ddsi_iid.h"
#include "cyclonedds/ddsi/ddsi_tkmap.h"
#include "cyclonedds/ddsi/ddsi_serdata.h"
#include "cyclonedds/ddsi/ddsi_threadmon.h"
#include "cyclonedds/ddsi/q_entity.h"
#include "cyclonedds/ddsi/q_config.h"
#include "cyclonedds/ddsi/q_gc.h"
#include "cyclonedds/ddsi/q_globals.h"

For comparison the shortened version:

#include "cydds/ddsi/ddsi_iid.h"
#include "cydds/ddsi/ddsi_tkmap.h"
#include "cydds/ddsi/ddsi_serdata.h"
#include "cydds/ddsi/ddsi_threadmon.h"
#include "cydds/ddsi/q_entity.h"
#include "cydds/ddsi/q_config.h"
#include "cydds/ddsi/q_gc.h"
#include "cydds/ddsi/q_globals.h"

Subjectively, it seems more organic. Although from the point of view of the file system, a directory named "cyclonedds" is certainly more informative. Are there any other naming options?

Oct 23 '19 20:10 i-and

Actually,

I would like to propose - what I would say is - a more elegant solution. It also might speed up the build on slower systems.

It's a dependency approach.

Each source-file (.c) has a public interface (.h), and potentially private or implementation-only interfaces (potentially in a different directory).

The public interface has potentially other dependencies, which need to be included first, and the same may apply to the private header file. The source file itself may have to pull in even more dependencies.

Keeping track of order of includes and even the path to the includes is cumbersome, tedious, error prone and inflexible. Worse, if you update or change the directory layout, you need to change all occurrences of the paths to point to the right directories.

Cue the dependency file. Every header-file uses special #defines to refer to the headerfiles it needs (you could extend this to be even more granular: just #define the types you need, but let's do the simpler case first)

So, if for example I need the ddsi_iid.h file, I would first create a dependency file:

#define D_CYCLONEDDS_DDSI_IID_H #define D_CYCLONEDDS_DDSI_TKMAP_H

Similarly, for private headerfiles:

#define D_CYCLONEDDS_PRIVATE_SOMEPRIVATE_H

Then, the master-dependency file is as follows:

#ifdef D_SOMEHEADER_FILE #define D_OTHER_DEPENDENCY_NEEDED_BY_THIS_FILE ... #endif

The order is from more dependent to less dependent.

The second half is then:

#ifdef CYCLONEDDS_DDSI_IID_H #include "<path/to/ddsi_iid.h" #endif

and so on, for every define one include file.

This ensures that all headerfiles are included exactly once. Of course, if multiple files depend on the same headerfile, the dependency will be specified multiple times, but this does not mean reparsing of the headerfile.

So, headerfile, will have corresponding .d file with dependencies (including the dependency on the headerfile itself) Sourcefile will include the .d file, specify additional dependencies, then include the master .d file, which will ensure that all necessary headerfiles will be pulled in.

The nice thing is that it is possible to generate the master-file and the .d automatically from current source. I should have some scripts somewhere that do this, but I am sure Erik B would be able to whip something up in perl before I could find the stuff I wrote in 1991. You might find it in the source-distribution of Motif++ from the same era, if it still is around somewhere.

Ronald

Ronald On 23/10/2019 22:21:56, i-and [email protected] wrote: From the point of view of the program code the prefix length exceeds the length of all subsequent components: #include "cyclonedds/ddsi/ddsi_iid.h" #include "cyclonedds/ddsi/ddsi_tkmap.h" #include "cyclonedds/ddsi/ddsi_serdata.h" #include "cyclonedds/ddsi/ddsi_threadmon.h" #include "cyclonedds/ddsi/q_entity.h" #include "cyclonedds/ddsi/q_config.h" #include "cyclonedds/ddsi/q_gc.h" #include "cyclonedds/ddsi/q_globals.h" For comparison the shortened version: #include "cydds/ddsi/ddsi_iid.h" #include "cydds/ddsi/ddsi_tkmap.h" #include "cydds/ddsi/ddsi_serdata.h" #include "cydds/ddsi/ddsi_threadmon.h" #include "cydds/ddsi/q_entity.h" #include "cydds/ddsi/q_config.h" #include "cydds/ddsi/q_gc.h" #include "cydds/ddsi/q_globals.h" Subjectively, it seems more organic. Although from the point of view of the file system, a directory named "cyclonedds" is certainly more informative. Are there any other naming options? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [https://github.com/eclipse-cyclonedds/cyclonedds/pull/284?email_source=notifications&email_token=ABMJNXBPCB27OSI2PFHANITQQCW5VA5CNFSM4JCRWLC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECCX3MI#issuecomment-545619377], or unsubscribe [https://github.com/notifications/unsubscribe-auth/ABMJNXCL6LVKGL2DNRGCVPTQQCW5VANCNFSM4JCRWLCQ].

Oct 23 '19 22:10 rvloon

A couple thoughts:

The length of an include doesn't really matter as long as it fits on a line. You're using one line for includes anyway.
There are too many headers. I think reducing the number of public headers would do a lot for readability, especially since so few are needed: https://github.com/ros2/rmw_cyclonedds/search?q=%22include+dds%22&unscoped_q=%22include+dds%22 afaict most apps just need dds.h
I think a macro system is overkill. If something is dependent on include order, that seems like a bug in the headers.

Oct 24 '19 00:10 rotu

I'm with @rotu here, especially because the only header file directly included by the vast majority of application code is (what is today) dds/dds.h. If that changes to cyclonedds/dds.h I don't think anyone would be upset about the increase in length. The #includes in the sources of Cyclone itself would get longer, but that is a small price to pay for having a clear prefix for the files.

One could argue that simply cyclonedds.h (and avoiding the repetition of "dds") would be a better option still. All other files would then be under a cyclonedds directory sitting next to it. I.e.:

cyclonedds.h
cyclonedds/ddsrt/sync.h
cyclonedds/ddsi/...

I suspect that would be trickier with auto-generating the redirects, though.

Outside the stable API (as included by dds.h) there are a ton of files that allow access to various internal functions. Some of these constitute things that I expect to evolve a bit before becoming stable interfaces for special uses (like custom topic/type implementations, custom reader history caches, &c. — those are the other ones you see in rmw_cyclonedds), some are just there so that a quick PoC can be done using some of the things in the library (e.g., it can be quite convenient to have access to AVL trees and hash tables).

The rest is just there because the special-use interfaces files still have dependencies on tons of internal things as a consequence of the long history of some the code and trying to install the minimal set is only extra work. And because I can point to Unix' /usr/include/sys mess for precedent 😁.

Computing the header file names with macros would make a rather big mess that confuses everyone and everything ... it's a good trick for the IOCCC and for people determined to build a template library for C, but otherwise not very practical.

While we're on the subject of renaming things: @rotu and @i-and, wouldn't you agree that renaming the library to "cyclonedds" (so, e.g., libcyclonedds.so) makes sense?

Oct 24 '19 07:10 eboasson

I suspect that would be trickier with auto-generating the redirects, though.

It wouldn’t be tricky at all to rename headers. Changing the library name would be hard, and I’m not sure how to do it gracefully. I do think it’s a good idea, though.

Oct 24 '19 16:10 rotu

While we're on the subject of renaming things: @rotu and @i-and, wouldn't you agree that renaming the library to "cyclonedds" (so, e.g., libcyclonedds.so) makes sense?

Your proposed renaming of the library seems logical.

Oct 24 '19 17:10 i-and

Changing the library name would be hard, and I’m not sure how to do it gracefully. I do think it’s a good idea, though.

On Linux/macOS we could do a soft link, but on Windows I have no clue either. Those who use CMake properly never need to type the name, so if there is a way of deleting ddsc (or refusing to install if it exists) the consequence would be simply that old executables would fail to run.

I think that's a price worth paying for cleaning up the naming.

Oct 25 '19 08:10 eboasson

I finally accepted that there would have to be a big notice recommending people to throw away the "old" ddsc libraries and all would be well. Now, I find myself in a dispute with CMake once again.

Renaming the library is as straightforward as replacing essentially all occurrences of "ddsc" related to the library by "cyclonedds" in the CMake files. It seems like leaving an alias target like this:

add_library(${PROJECT_NAME}::ddsc ALIAS cyclonedds)

(so basically the old one) should allow application CMake files to continue to refer to it as CycloneDDS::ddsc, but it doesn't seem to work. I must be misreading the CMake documentation:

add_library(name ALIAS target)

Creates an Alias Target, such that name can be used to refer to target in subsequent commands. The name does not appear in the generated buildsystem as a make target. The target may not be a non-GLOBAL Imported Target or an ALIAS. ALIAS targets can be used as linkable targets and as targets to read properties from. They can also be tested for existence with the regular if(TARGET) subcommand. The may not be used to modify properties of target, that is, it may not be used as the operand of set_property(), set_target_properties(), target_link_libraries() etc. An ALIAS target may not be installed or exported.

Or would it be ok to force everyone to update their CMake files? But if that's the case, then why even bother with providing forwarding header files for backwards compatibility?

Anyone has a better idea?

(See https://github.com/eboasson/cyclonedds/tree/pull-284)

Nov 06 '19 14:11 eboasson