cbm-fles / flesnet

CBM FLES Timeslice Building
7 stars 22 forks source link

System.hpp duplication #108

Open cuveland opened 1 year ago

cuveland commented 1 year ago

"The System.hpp header is duplicated in monitoring and fles_ipc, once in the cbm namespace and once in the fles namespace. The one in monitoring seems to be newer with more functions. I am not sure how to solve this. I do not see a library that monitoring and fles_ipc both use where we could put the code."

This issue has been reported by @fweig.

cuveland commented 1 year ago

An obvious idea would be to put this into an independent "utility" library to be included by both. However, I wanted to avoid the additional complexity for now.

I feel that we have not yet arrived at the final build system structure in CBM computing, especially with respect to online computing and the interaction between flesnet and cbmroot. More specifically, there are things currently in the flesnet repository that I see more in a common support library, such as the code for monitoring, logging, and also these helper functions.

So my take would be to postpone cleaning up this duplication for now. As we are indeed in different namespaces, it should not cause any immediate problems, and if both are included, I would opt for using the cbm namespace version by default.

@fweig, @oTTer-Chief: What do you think?

fweig commented 1 year ago

I agree, that creating a separate library is overkill for now. And it's ok to keep the code duplication for now.

However there is one problem remaining: With my recent changes in CbmRoot (!1180, I have to install both the monitoring and fles_ipc libraries with CbmRoot to appease the CI. This creates the problem that both System.hpp need to be installed in parallel, which isn't possible at the moment. As one will just overwrite the other one. I could try to modify my MR to bypass installing monitoring. But this just postpones the issue, imo.

As a simple workaround, I'd suggest to rename the header in fles-namespace to "FlesSystem.hpp". This resolves the naming conflict and makes it clearer that the other one should be used more broadly.

cuveland commented 1 year ago

In fact, the issue that identically named header files can potentially create a problem is already present in flesnet as it is. But in the long run, as everything grows, I feel that we cannot assume that all include files are named differently. I think a fair way to address this would be to include the directory/library names in the #include directives and use the /lib directory as the include base. As this would require changing almost all files in the repository, we did not implement it yet.

As for the inclusion in cbmroot, I did not yet fully understand why we need to "install" the header files, especially if we only use them in compiled code.

To my understanding, there is a large difference between what we call a "library" in CMake (basically a way of structuring the source code) and what a real system-level library is (a self-contained project to be used independently in different programs). Flesnet currently uses the former. Thus, there is support for installing the built applications, but not the internal "libraries".

Correctly going for installable libraries would first of all mean a clean separation between interface and implementation. Typically, there are two kinds of header files: Those describing the interface, which are installed in /usr/include, and internal ones, which are only used when generating the compiled libraries in /usr/lib. And there is a more complex directory structure involved (which also solves any issues with identical file names). We currently do not have this, and I think it would involve serious refactoring.

My idea of including the flesnet repository into cbmroot as we do now was that we could handle it somehow like a large joint CMake project with CMake-style "libraries" in a bunch of folders and foresee only the build results for installation.