Skip to content

Conversation

@MicK7
Copy link
Contributor

@MicK7 MicK7 commented Jan 18, 2022

Proposed Changes

Give a brief overview of your contribution here in a few sentences.
The proposed changes consist in providing an hdf5 enabled cgns library inside SU2.
This is done by embedding the needed hdf5 sources inside the cgns directory.
A basic meson.build file helps with compilation (instead of original cmake).

Related Work

None

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

MicK7 added 2 commits January 18, 2022 20:23
- hdf5 1.12 source copied in cgns
- add cgns files from 4.2 to support hdf5 backend
- add parallel cgns build detection to meson
@MicK7
Copy link
Contributor Author

MicK7 commented Jan 18, 2022

The branch can be tested by converting an existing testcase cgns file to hdf5.

@MicK7 MicK7 force-pushed the su2_cgns_with_hfd5 branch from 46bb9fe to 99dd297 Compare January 18, 2022 21:26
@MicK7 MicK7 requested review from pcarruscag and talbring January 19, 2022 15:45
@MicK7
Copy link
Contributor Author

MicK7 commented Jan 19, 2022

It seems that LGTM analysis is using a very old version of meson. Can someone update it ?

@pcarruscag
Copy link
Member

Thanks for the update @MicK7, this only affects code in externals right? if so there is no need to worry about LGTM, I think it was using the old build system.

@MicK7
Copy link
Contributor Author

MicK7 commented Jan 19, 2022

@pcarruscag yes all the modification are in external. It is just that the LGTM C++ if failing because it is too old and can't cope with cmake syntax. Since I mainly copy pasted the source of hdf5 without modification, I don't really want to modify them for maintainability.

@MicK7 MicK7 closed this Jan 19, 2022
@MicK7 MicK7 reopened this Jan 19, 2022
@MicK7
Copy link
Contributor Author

MicK7 commented Jan 19, 2022

My request to update the meson currently used by LGTM C/C++ is to not create issues for other PR is this one goes in.

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I got that, I was asking to know if I should look at any of the 531 files 😄
LGTM seems fixed.

@pcarruscag
Copy link
Member

Does this support a type of CGNS that we didn't before? If so can you add a small test?

@MicK7
Copy link
Contributor Author

MicK7 commented Jan 19, 2022

@pcarruscag I can change one of the existing testcases to have a validation

@MicK7 MicK7 merged commit 71a0942 into develop Jan 19, 2022
@MicK7 MicK7 deleted the su2_cgns_with_hfd5 branch January 19, 2022 23:33
@pr-triage pr-triage bot added the PR: merged label Jan 19, 2022
@jtlau
Copy link
Contributor

jtlau commented Jan 21, 2022

I'm having an issue compiling on macOS. I get an error after "./meson.py build":

Configuring H5pubconf.h using configuration

externals/cgns/hdf5/meson.build:662:0: ERROR: #mesondefine does not contain exactly two tokens: #cmakedefine H5_HAVE_WINDOWS @H5_HAVE_WINDOWS@

I'm not sure where to go from there... since I don't need cgns I workaround this by disabling it...
LMK if you need the full configure error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants