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

Restructuring of the repository #90

Open
efokken-abb opened this issue Aug 11, 2023 · 4 comments
Open

Restructuring of the repository #90

efokken-abb opened this issue Aug 11, 2023 · 4 comments

Comments

@efokken-abb
Copy link

Hi!

As my name suggests I'm working for ABB with this library.
In order to understand how to work with the library, I quite heavily restructured the repository and decomposed externalmedialib it into different sub-libraries that are all linked into externalmedialib.

My question is the following:
Would you possibly be interested in a PR containing the restructuring? I'll describe it in the following and would like some idea on how willing you would be to merge it/advice me on possible revisions such that you would then like to merge it, before I start the process of getting formal approval from my company for contributing the restructuring back to the project.

The pull request would in total contain the following:

  1. Project/Sources is decomposed into sub-libraries that are statically linked into libExternalMediaLib.so (or .dll etc.). These sublibraries have their dependencies described in their own CMake files.
  2. CoolProp is built and linked as a static library, so that this project can simply use coolprop's public include directories published by the CoolProp Cmake target instead of tracking coolprop's include directories manually.
  3. CoolProp is made (again, if I understand correctly) a git submodule. This also reduces the minimum cmake version to 3.9 (see Wrong version of cmake required to compile the library #89 and the next point, right now it is apparently 3.18)
  4. To reclaim the performance that is lost by introducing static libraries instead of compiling everything together, cmake is instructed to check for IPO capabilities of the compiler (a.k.a. link-time-optimization) an activate them in case they are found. This raises the minimum CMake version to 3.9 (Without IPO it would be 3.8, because of the generator expression
target_compile_definitions(${LIBRARY_NAME} PRIVATE EXTERNALMEDIA_FLUIDPROP=$<IF:$<BOOL:${FLUIDPROP}>,1,0>)

)
5. Symbols are hidden by default and only those also exported under Windows are visible.
6. The outer usage would stay the same, apart from needing to clone the repository with --recursive because of the git submodule. Note that I don't have access to FluidProp, so can not test it.

Advantages are in my opinion the following:

  1. Clear dependency structure of the different parts of the library make it easier to reason about it and probably easier to maintain it in the long run.
  2. Easier to add unit tests (because of the smaller sub-libraries).
  3. Easier to track coolprop, because no knowledge of its internals is needed.

Disadvantages:

  1. Very old compilers may not support IPO and therefore there may be some performance drop. I haven't measured anything but would be surprised if this was a big drop.
@jowr
Copy link
Collaborator

jowr commented Aug 15, 2023

I think this is a super nice opportunity. Please go ahead and prepare a PR - I will support the integration and I may also help with the CI part.

@efokken-abb
Copy link
Author

Nice, I'll try to get approval and will then check back with you.

@efokken-abb
Copy link
Author

Just a quick heads-up: Any news on this will not come before mid-September.

jowr added a commit that referenced this issue Sep 5, 2023
- This closes #89 and possibly relates to #90. 
- The new manually triggered CI job uses the suggestions from #89 to find the minimum CMake version.
- Wrong version of cmake required to compile the library #89 to find the minimum CMake version.
- The minor adjustments in the CMakeLists.txt reduce the needed CMake version to 3.8 instead of 3.18
- The missing CoolProp include directories have also been fixed.
- As pointed out in #85, we may want to bring back the subdirectory notation for GCC and friends, this is also included here.
@efokken-abb
Copy link
Author

Another heads-up: The application for open-sourcing the restructure is now in the process of approval. Unfortunately I cannot give a further time estimate but will of course report back as soon as I know more.

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

No branches or pull requests

2 participants