Skip to content

Conversation

@bartgol
Copy link
Contributor

@bartgol bartgol commented Jul 23, 2025

Updates HAERO to use the new package-based version of ekat.

@jeff-cohere I am not 100% sure if what I did wrt to yaml/spdlog is correct; the cmake logic on that regard seemed "long". In particular,

  • I no longer enable spdlog: I did not see it used in HAERO, but maybe you want it enabled b/c you want to install haero+ekat for later use?
  • I always enable yaml-cpp: I removed handling of HAERO_SKIP_FIND_YAML_CPP.

In both cases, I figured you can just invoke haero's config passing EKAT_XYX=ON/OFF vars, and get the corresponding option in ekat triggered. Here, I only hard-coded ON/OF for what Haero knows for sure it's going to need...

@bartgol
Copy link
Contributor Author

bartgol commented Jul 24, 2025

@mjschmidt271 do you guys enforce clang formatting? Here it's complaining about an empty line between headers, but I see that's done also in other files. If you want, I can address it though.

Edit: actually, I thought I did address it, with the lat commit. I honestly have no idea what clang's complaint is about... Do you know what it wants me to do? If not, then I'll remove the last commit, which is imho pointless (but I thought that was what clang complained about)

@mjschmidt271
Copy link
Contributor

@bartgol We do enforce, and it looks like its primary issue is with the ordering of the ekat headers not being alphabetical. It also deletes the empty line, but that appears to be a non-necessary side effect

@bartgol
Copy link
Contributor Author

bartgol commented Jul 24, 2025

Ok, I can make the includes alphabetical.

BTW, I don't think that's a good rule: headers should be included from most specific to most generic, to avoid hiding includes problems. E.g., if header B.hpp uses stuff from A.hpp but does not include it, it is ill-formed. But if these two are currently only used in C.hpp and included alphabetically, the error is hidden. Unfortunately, a formatter cannot check this.

That said, I will update the formatting.

@bartgol bartgol force-pushed the bartgol/ekat-update-0519025 branch from dcd893c to a84eff8 Compare July 24, 2025 19:26
@mjschmidt271 mjschmidt271 self-requested a review July 25, 2025 18:05
Copy link
Contributor

@mjschmidt271 mjschmidt271 left a comment

Choose a reason for hiding this comment

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

Looks good--thank you for taking care of this!

@bartgol bartgol merged commit 83a7d0d into eagles-project:main Jul 28, 2025
5 checks passed
@bartgol bartgol deleted the bartgol/ekat-update-0519025 branch July 29, 2025 15:34
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.

2 participants