mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
103 stars 58 forks source link

Closes #190 and #191 #192

Closed mgoodson-cvd closed 2 years ago

mgoodson-cvd commented 2 years ago

Closes #190. Fixes an issue with Fortran compiler flags not being set properly.

Closes #191. Fixes an issue with the Fortran interface install directories not being set properly.

rdbisme commented 2 years ago

Hello @mgoodson-cvd, thanks! I don't see the change to the install directories (I thought I had fixed it). Also no need to cache compiler flags.

rdbisme commented 2 years ago

Also if you can it might be cool to also enable the build of the Fortran wrapper in the CI checks such as to catch issues related to Fortran in the CI

codecov[bot] commented 2 years ago

Codecov Report

Merging #192 (9113492) into master (c9a924f) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #192   +/-   ##
=======================================
  Coverage   71.22%   71.22%           
=======================================
  Files         135      135           
  Lines        8972     8972           
=======================================
  Hits         6390     6390           
  Misses       2582     2582           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c9a924f...9113492. Read the comment docs.

mgoodson-cvd commented 2 years ago

I'm not changing the behavior of the install directories; I'm changing when CMake steps into the Fortran subdirectory because where it is now is too early; the Fortran CMake tries to use INSTALL_LIB_DIR and INSTALL_INCLUDE_DIR but they haven't been defined yet in the main CMakeLists.txt. I just moved the call to add_subdirectory(interface/fortran) to after these variables have been set.

rdbisme commented 2 years ago

I merged the fix on the install directories with #193. If you manage to check if without caching anything, the flags are correctly shown in the GUI after the second run of cmake, then I'd prefer to not cache them. If not, we will.

In both cases feel free to open another PR to address this :)

mgoodson-cvd commented 2 years ago

@rubendibattista I just pulled master after your merge of #193 and tried configuring using the CMake GUI (ccmake). With the compiler flags not cached, they are not shown correctly in the GUI. Also note the absence of the CMAKE_CXX_FLAGS_PROFILE, etc. after the changes in ab639ae0; because they are no longer cached, they aren't shown in the GUI.

image

As before, this is just cosmetic; the actual compiler flags are set properly. The confusing part would be if the user tried to modify these flags via the GUI, they wouldn't actually be modified (I just tested this). And yes, you do have to "configure" multiple times in the GUI, otherwise the "generate" option never appears and you can't build -- you can see I did that properly in the screenshot.

I will open a new PR to cache the Fortran compiler flags and re-cache the C++ compiler flags.