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

New code structure #100

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

efokken-abb
Copy link

@efokken-abb efokken-abb commented Jan 4, 2024

Finally I could finish this (addresses #90)

Note that the commits are a bit confused. So I explain what I did, here.
Many tests fail, unfortunately. For Windows, FluidProb is needed, which I don't have. For MacOS, something with the sprintf function is a problem, which I, if I remember correctly, didn't touch.

So, here it is:

Complete restructure of the Cmake structure of ExternalMediaLib

  1. The Sources directory was replaced by a directory ExternalMediaLib.

  2. The sources of ExternalMediaLib are now grouped into different static
    libraries with clear interdependencies, such that adding a new external solver
    should be straight-forward: To do this, you need to

    a)add a new directory Solver<Name_of_your_external_library> inside
    Projects/ExternalMediaLib

    b) copy the CMakeLists.txt from SolverCoolProp and
    change it to your needs

    c) Define a new cmake variable (like COOLPROP) and add building logic to
    Projects/CMakeLists.txt

    d) Conditionally on this variable include and link
    Solver<Name_of_your_external_library> inside
    Projects/ExternalMediaLib/CMakeLists.txt

    e) Conditionally on this variable include your header file (best located in
    Solver<Name_of_your_external_library>/include in solvermap.cpp and add
    corresponding code to instantiate your solver in solvermap.cpp

  3. In addition CoolProp is now a static library, so that dependencies to it can
    be managed with CMake, instead of manually in the ExternalMediaLib
    CMakeLists.txt.

  4. Note that the FluidProp interface is untested, because I have no license for it. For this reason continuous integration checks for MSVC fail.

  5. The symbols exported from externalmedialib.so when compiling and linking with
    clang or gcc are now only those also exported by msvc, so that it is safe to
    link to another instance of the coolprop library and in general have symbols
    defined that are named as those in externalmedialib.

  6. There is now the cmake option SUPPRESS_IPO, which defaults to OFF. If it is OFF, link time optimization is carried out, making compilation (or rather linking) longer, while making the code probably faster/smaller.

  7. There are two additional libraries ModelicaStaticHelper and ModelicaUtilities. These are added, so that symbol overloading for ModelicaError and ModelicaWarning work. This way, one can compile the externalmedialib without linking the modelica standard library.

Please let me know, what you think.

This means that we can just rely on coolprop to pass on its dependencies,
instead of tracking possible changes in them.
The submodule is right now checked out to the latest stable version of
coolprop, namely 6.4.3.
This way, the failure to implement needed methods on base solver
is caught at compile time, instead of runtime.
Also moved the docs in the header file.
These do have default implementations.
In all methods that are not actually implemented, an error is reported.
…ld and ld.

The MSVC linker should include only explicitly exported symbols, so
it doesn't need this (not tested!). All other linkers are not really supported here,
unless they behave like the MSVC linker.
Also: Make the ModelicaUtilisties library that is included, so the test
can be built a shared library.
1. The Sources directory was replaced by a directory ExternalMediaLib.

2. The sources of ExternalMediaLib are now grouped into differenct static
libraries with clear interdependencies, such that adding a new external solver
should be straight-forward: To do this, you need to

    a)add a new directory Solver<Name_of_your_external_library> inside
    Projects/ExternalMediaLib

    b) copy the CMakeLists.txt from SolverCoolProp and
    change it to your needs

    c) Define a new cmake variable (like COOLPROP) and add building logic to
    Projects/CMakeLists.txt

    d) Conditionally on this variable include and link
    Solver<Name_of_your_external_library> inside
    Projects/ExternalMediaLib/CMakeLists.txt

    e) Conditionally on this variable include your header file (best located in
    Solver<Name_of_your_external_library>/include in solvermap.cpp and add
    corresponding code to instantiate your solver in solvermap.cpp

3. In addition CoolProp is now a static library, so that dependencies to it can
be managed with CMake, instead of manually in the ExternalMediaLib
CMakeLists.txt.

4. Note that the FluidProp interface is untested, because I have no license for it.

5. The symbols exported from externalmedialib.so when compiling and linking with
clang or gcc are now only those also exported by msvc, so that it is safe to
link to another instance of the coolprop library and in general have symbols
defined that are named as those in externalmedialib.

6. The next commit will introduce a check for inter process optimization (a.k.a.
link time optimization) to remove the possible runtime penalties introduced by
the sub-division into static libraries.
…lation

This of course reduces performance and should not be used for building
binaries for production usage.
also: explained the existence of the targets modelica_static_helper and
modelica_utilities_DONT_USE

in their CMakeLists.txt files.
(different function signatures between declaration and implementation.
@efokken-abb
Copy link
Author

About the confusing git history of this pull request:
At first I changed some things in the basesolver and declared almost all methods there virtual.
Afterwards I realized that usually not all these methods are actually overridden by child classes and reverted this.

Another example is: At first I tried to hide all non-explicitely exported symbols via another cmake functionality, which in the end didn't really work. So then I instead defined an export macro similar to the one used by MSVC and used hiding symbols by default via https://cmake.org/cmake/help/v3.0/prop_tgt/LANG_VISIBILITY_PRESET.html

A remnant of this journey is the commit named
Exclude all statically linked libraries.

@jowr
Copy link
Collaborator

jowr commented Jan 8, 2024

Thank you very much - it may take some time to review the changes, but your effort is highly appreciated!

@efokken-abb
Copy link
Author

No worries, I myself took much longer than I anticipated. If you have questions/need me to revise something, please get back to me!

@eike-fokken
Copy link

Hi! Any news on this?

@casella
Copy link
Collaborator

casella commented May 14, 2024

@efokken-abb sorry, my teaching semester will finish in two weeks, then I hope I have some time to look into this. Thanks!

@efokken-abb
Copy link
Author

Hi! Did your semester conclude? And do you still have time for this?

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

Successfully merging this pull request may close these issues.

4 participants