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

Copy only necessary cmake macros to case #4492

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

jgfouca
Copy link
Contributor

@jgfouca jgfouca commented Sep 25, 2023

This implementation makes some assumptions about the context of /.../Macros.cmake, so I am only turning this on for E3SM.

I think this is a nice improvement. This will reduce the number of macros that get copied from nearly 100 to just a handful, making it easier for the user to view and edit their macros from the case.

Test suite: by-hand
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes [CIME Github issue #]

User interface changes?:

Update gh-pages html (Y/N)?:

This implementation makes some assumptions about the context
of /.../Macros.cmake, so I am only turning this on for
E3SM.
@jgfouca jgfouca self-assigned this Sep 25, 2023
Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

I don't see anything here that limits this to e3sm. Please just add two more files and this will apply to both models.


# This impl is coupled to contents of Macros.cmake
macros = [
"universal.cmake",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you should include cesm. To do that you just need to add a couple more files:
${OS}.cmake
${COMPILER}_${OS}.cmake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @jedwards4b , I am happy to do so. I don't have access to you guys' Macros.cmake file, so I didn't know for sure if it had been changed to load different patterns. I will change it so that it works for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

our Macros.cmake file is generated in cime/CIME/build.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jedwards4b , are you sure? I can see where the Makefile macros are generated (from the Cmake ones) but I think the cmake ones need to exist already:

        cmake_macro = os.path.join(caseroot, "Macros.cmake")
        expect(
            os.path.exists(cmake_macro),
            "Cannot generate Makefile macro without {}".format(cmake_macro),
        )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - I just tried a build with this PR and it works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jedwards4b , great. So do you approve of this PR now?

@jgfouca jgfouca merged commit b8d6d2b into master Sep 26, 2023
9 checks passed
@jgfouca jgfouca deleted the jgfouca/macros_copy_only_needed branch September 26, 2023 19:41
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