[CoSim] Changing CMAKE to C++17 in CoSIm External Lib

[Igarizza]

📝 Description
During compilation on HPC we found that cosim appliction can be compiled only with c++11, which caused errors in our compilation. Changing cmake file to use c++17 helped.

[Igarizza]

@matekelemen FYI

[philbucher]

Hey, not against but would like to know the errors

Perhaps there is a better fix

[matekelemen]

I never tried compiling parts of the same program with different standards, but I wouldn't be surprised if it were dangerous.

One of the errors I remember when we tried compiling on the cluster is that std:filesystem namespace was (obviously) not found.

P.S.: here

Kratos/applications/CoSimulationApplication/custom_external_libraries/CoSimIO/co_sim_io/includes/filesystem_inc.hpp

Line 17 in 08507c7

namespace fs = std::filesystem;

[philbucher]

I agree that this has to be improved, I wonder why I did it like this in the first place.

And how it was working on otter systems.

I need to think a bit

[Igarizza]

I never tried compiling parts of the same program with different standards, but I wouldn't be surprised if it were dangerous.

One of the errors I remember when we tried compiling on the cluster is that std:filesystem namespace was (obviously) not found.

P.S.: here

Kratos/applications/CoSimulationApplication/custom_external_libraries/CoSimIO/co_sim_io/includes/filesystem_inc.hpp

Line 17 in 08507c7

namespace fs = std::filesystem;

/home/ihaanton/Software/Kratos/applications/CoSimulationApplication/custom_external_libraries/CoSimIO/co_sim_io/includes/filesystem_inc.hpp:17:21: error: ‘filesystem’ is not a namespace-name; did you mean ‘system’?
   17 | namespace fs = std::filesystem;
        |                     ^~~~~~~~~~
        |                     system

[philbucher]

I never tried compiling parts of the same program with different standards, but I wouldn't be surprised if it were dangerous.

not sure I follow. Different programs can use different compilers, standards, even languages ...?
E.g. feast (eigensolver) which is written in fortran

[philbucher]

since I manually removed the filesystem bcs Kratos is C++17, I think it is also ok to hack it like this. I will keep it in mind, maybe I come across a smarter/automatic solution

[matekelemen]

not sure I follow. Different programs can use different compilers, standards, even languages

Sure, but as always it's a bit difficult with C++. I'm by no means an expert at this, but as far as I know you must keep a few things in mind when you want to mix standards/compilers/etc:

First that comes to mind is never to pass standard classes through API boundaries, because the binary layout may be different across programs if compiled with different compilers. For example

dependency.hpp

#include <vector>
void dependency(std::vector<int>& arg);

dependency.cpp

void dependency(std::Vector<int>& arg)
{
    // do some stuff with arg
}

user.cpp

#include "dependency.hpp"
#include <vector>
int main()
{
    dependency(std::vector<int> {});
}

if dependency is a library compiled with GCC and user is a program compiled with Clang, they may have a different implementation of vector and have different binary layouts as a result, i.e.: when user constructs and passes a vector, dependency will receive a vector that is corrupt (from its point of view).

Another problem is which headers to include. user.cpp includes GCC vector, but it should also indirectly include Clang vector via dependency.hpp, but there must only be one definition of std::vector.

These are just some things that come to mind, and CoSimIO is definitely affected, as the main interface header includes standard headers and also puts standard classes on its API:

Kratos/applications/CoSimulationApplication/custom_external_libraries/CoSimIO/co_sim_io/co_sim_io.hpp

Line 25 in 1166a00

#include "includes/model_part.hpp"

Kratos/applications/CoSimulationApplication/custom_external_libraries/CoSimIO/co_sim_io/includes/model_part.hpp

Lines 24 to 28 in 1166a00

	#include <vector>
	#include <string>
	#include <unordered_map>
	#include <atomic>
	#include <ostream>

https://github.com/KratosMultiphysics/CoSimIO/blob/a1feb8fe6e1ca7ba42e56c39202aa9e519129ea8/co_sim_io/sources/co_sim_io.cpp#L68-L72

P.S.: the same issues are still valid with the same compiler but different standards. Some standards might retain binary compatibility, but they will almost certainly have different headers.

[philbucher]

Thanks for the explanation, that makes sense

I think trilinos, metis etc have a C API, guess for this reason

For me still unclear is how it works e.g. in the CI bit not ok your machines

Kratos sets c++17 as standard, do you have a clue why it is not applied go CoSimIO? It is a subdir, so it should IMO. Maybe it is the order of compilation?

@roigcarlo perhaps you have an idea?

[matekelemen]

For me still unclear is how it works e.g. in the CI bit not ok your machines

I'm also surprised about that

Kratos sets c++17 as standard, do you have a clue why it is not applied go CoSimIO?

Because CoSimIO explicitly sets the C++11 standard via compiler flags (see the changes in this PR). I think we could just remove the -std=c++xx flag, and that would allow the parent project (Kratos) to set its desired standard (17).

The problem is what happens when someone wants to compile CoSimIO on its own? Then CoSimIO would have to set the minimum version it requires (11). I suggest adding a CMake flag for CoSimIO that controls what the preferred C++ standard should be, whose default value is 11.

[roigcarlo]

I will investigate, but as you said, if the CoSim sets the flat to 11, and it get added after the Kratos one, it will use 11 regardless of what Kratos says.

[matekelemen]

@roigcarlo any updates?

I think this should be fixed upstream, and then ported to kratos. Any objections? Should I do it?

[roigcarlo]

No so far sorry :/ but I would merge the PR if that solves the issue.