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.

Sign up for GitHub

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

Jump to bottom

Uppercase ESMF_ROOT for Spack #768

Closed
wants to merge 6 commits into from
Closed

Uppercase ESMF_ROOT for Spack #768

wants to merge 6 commits into from

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Mar 7, 2024 •
edited
Loading

Closes #767

Per @climbfuji, we should use ESMF_ROOT instead of esmf_ROOT as that is more conventional.

This does not affect GEOS-with-Baselibs runs as we are not in this block.

Note, I've also uppercased the target to ESMF as that is what ESMF itself provides. Eventually we should change this to ESMF::ESMF but that will need to wait for ESMF 8.6.1

@mathomp4 mathomp4 added the 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label Mar 7, 2024
@mathomp4 mathomp4 self-assigned this Mar 7, 2024
@mathomp4 mathomp4 requested review from a team as code owners March 7, 2024 21:15
@climbfuji
Copy link

Thanks @mathomp4 ! Maybe we can wait merging this until the new spack-stack using the uppercase version is rolled out?

@mathomp4
Copy link
Member Author

mathomp4 commented Mar 7, 2024

@climbfuji If you can test it to make sure it works, that's great, but we can hold off as long as you'd like as it doesn't affect our Baselibs builds.

Once you need it let us know.

@mathomp4 mathomp4 marked this pull request as draft March 7, 2024 21:20
@mathomp4
Copy link
Member Author

mathomp4 commented Mar 7, 2024

I'll mark this draft to make sure we don't merge until wanted.

climbfuji
climbfuji approved these changes Mar 7, 2024
View reviewed changes
Copy link

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Works for me, thanks very much! I'll let you know when the time is ready to merge.

@climbfuji
Copy link

Uhh, didn't we revert this uppercase change because of all the cmake warnings? And raised it with the spack developers?

@mathomp4
Copy link
Member Author

mathomp4 commented May 3, 2024

Uhh, didn't we revert this uppercase change because of all the cmake warnings? And raised it with the spack developers?

@climbfuji This has been kept draft for now. We can close it if you like. I wasn't sure if you still wanted it or not so... Up to you!

@climbfuji
Copy link

I checked on my mac, and we indeed reverted to case-preserving packageName_ROOT. Let's close this. Sorry for the hassle!

@mathomp4
Copy link
Member Author

mathomp4 commented May 3, 2024

I checked on my mac, and we indeed reverted to case-preserving packageName_ROOT. Let's close this. Sorry for the hassle!

Eh. Wasn't exactly a hard PR to make :) Closing!

@mathomp4 mathomp4 closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@climbfuji climbfuji climbfuji approved these changes

Assignees

@mathomp4 mathomp4

Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Need to search for uppercase ESMF_ROOT instead of esmf_ROOT in CMakeLists.txt with next spack-stack release
2 participants
@mathomp4 @climbfuji