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

Update rrfs-test/CMakeLists.txt to be more compact for adding a CTest #81

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

SamuelDegelia-NOAA
Copy link
Contributor

@SamuelDegelia-NOAA SamuelDegelia-NOAA commented Jun 14, 2024

This PR updates RDASApp/rrfs-test/CMakeLists.txt to be a little more user-friendly by making it easier to add CTests. Two new functions are added to allow for a “dictionary-like” data structure in CMake. To add a new CTest now, you just need to add a single line to either the fv3-jedi or mpas-jedi lists at the top of the file:

### Define CTests here ###

# FV3-JEDI tests
set(rrfs_fv3jedi_tests "")
add_to_dictionary(rrfs_fv3jedi_tests "rrfs_fv3jedi_hyb_2022052619"     "fv3jedi_var.x")
add_to_dictionary(rrfs_fv3jedi_tests "rrfs_fv3jedi_letkf_2022052619"   "fv3jedi_letkf.x")

# MPAS-JEDI tests
set(rrfs_mpasjedi_tests "")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_Ens3Dvar"     "mpasjedi_variational.x")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_atms_npp_qc"  "mpasjedi_variational.x")
add_to_dictionary(rrfs_mpasjedi_tests "rrfs_mpasjedi_2022052619_letkf"        "mpasjedi_enkf.x")

All CTests in RDASApp/build/rrfs-test pass after making this change.

Also, per discussion in #79, we will likely move to one mega executable in the near future (i.e., just a single rdas_jedi.x instead of fv3jedi_var.x, fv3jedi_letkf.x, mpasjedi_enkf.x, etc.). CMakeLists.txt will need to be updated again at that point, but we should be able to keep the same new code structure that is added in this PR. It will eventually look something like:

add_to_dictionary(rrfs_tests “rrfs_fv3jedi_hyb_2022052619” “fv3jedi” “variational”)

@guoqing-noaa
Copy link
Collaborator

I would think we probably do "over-engineering" here.
@SamuelDegelia-NOAA 's first solution in the issue #79 without a function is enough to handle the problem and is easy to read by any developers. More importantly, if we decide to use one single executable, this change will be replaced soon. I would think investing efforts in one single executable will be more valuable. My two cents.

@TingLei-NOAA
Copy link
Contributor

I would think we probably do "over-engineering" here. @SamuelDegelia-NOAA 's first solution in the issue #79 without a function is enough to handle the problem and is easy to read by any developers. More importantly, if we decide to use one single executable, this change will be replaced soon. I would think investing efforts in one single executable will be more valuable. My two cents.

I am really wondering when there would be a super executable to be realized ,and I also have some different opinions on such a practice. Second, even with such one exec being realized, I expect it needs an command option to tell what run it is . While that kind of options are , as Sam mentioned, treated with the help of the introduced dic-like data structure.

@SamuelDegelia-NOAA
Copy link
Contributor Author

I updated this PR to now use the function get_all_keys instead of regex. Hopefully this is good to go now.

Copy link
Contributor

@TingLei-NOAA TingLei-NOAA left a comment

Choose a reason for hiding this comment

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

@SamuelDegelia-NOAA the current version looks working cleanly with much improved readability by using the dic-like structure. Thanks.

@ShunLiu-NOAA ShunLiu-NOAA merged commit 20796ce into develop Jun 18, 2024
3 checks passed
@ShunLiu-NOAA ShunLiu-NOAA deleted the feature/clean_exelist_cmake branch June 18, 2024 15:24
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