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

Move makdep compilation to Makefile #257

Merged
merged 5 commits into from
May 9, 2019

Conversation

phil-blain
Copy link
Member

@phil-blain phil-blain commented Apr 24, 2019

  • Developer(s): Philippe Blain

  • Please suggest code Pull Request reviewers in the column at right. @apcraig

  • Are the code changes bit for bit, different at roundoff level, or more substantial? BFB

  • To verify that this PR passes the initial QC tests, please include the link to test results
    or paste in below the summary block from the bottom of the testing output.
    No tests, did not modify source code.

  • Does this PR create or have dependencies on CICE or any other models? No

  • Is the documentation being updated with this PR? (Y/N) Yes
    https://icepack-phil-blain.readthedocs.io/en/build-system-robustness/user_guide/ug_running.html#cross-compiling
    If not, does the documentation need to be updated separately at a later time? (Y/N) No

Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

  • Other Relevant Details:

* makdep compiled from a makdep rule in the Makefile
* Add CFLAG_HOST macro
* makdep is only compiled if needed
* This addresses CICE-Consortium/CICE#306
@phil-blain
Copy link
Member Author

phil-blain commented Apr 24, 2019

The Travis build failed because the Makefile macro SCC was not defined in the Macros.travisCI_gnu file. I think this file is a little outdated, as the macros SCC and SFC were there but were empty. I updated that.

Also, the Travis build was using the clang preprocessor, I think this is an historical artifact so I removed that and set CPP to /usr/bin/cpp instead in the Macro file. I also added gcc to the travis.yml file.
If I'm out of line with these changes I will revert them.

* This is similar to the machine files cleanup in CICE-Consortium/CICE#300
* Remove MPICC, MPIFC since there is no MPI in Icepack
@phil-blain
Copy link
Member Author

I updated all machine files to define correctly SCC, SFC, CC, FC, LD. I removed the references to MPI since it is not used in Icepack.

@ghost
Copy link

ghost commented Apr 25, 2019

Hi Phil, I used clang because it worked with some preprocessing directives where gcc did not. If these issues have been sorted out that's great. If you'd like, the Travis tests can be run in parallel with both gcc and clang, just to check beyond a single compiler.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Will test on a few other platforms then merge.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Minor point, could/should the Makefile rule for makdep be

$(DEPGEN): $(ICE_CASEDIR)/makdep.c

since we have defined DEPGEN earlier and use it in the Makefile. It seems for complete consistency, we should use DEPGEN here or think about getting rid of it and use "makdep" throughout. This is very minor, but might as well think about it while it's here.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Everything works fine on conrad, running a quick suite with 4 compilers. I also tested the change in the Makefile,

-makdep: $(ICE_CASEDIR)/makdep.c
+$(DEPGEN): $(ICE_CASEDIR)/makdep.c

and that works fine too. I will approve and merge once we decide whether to include this change.

@phil-blain
Copy link
Member Author

I agree that the external dependency generator is unlikely to change, but you are right that for consistency we can use $(DEPGEN) as the target.

@phil-blain
Copy link
Member Author

I just thought that the new make variable CFLAG_HOST should be documented. I will update the doc. Commit upcoming.

@phil-blain
Copy link
Member Author

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Looks great, thanks.

And thanks for cleaning up the Macros. That was needed. Originally, the CICE and Icepack scripts just used the same files, but we'd know they would diverge and they have. The CICE build has been updated as needed and the Icepack has sometimes been left behind. In addition, it was finally time to remove the stuff in the Icepack build that is not needed in Icepack (but was for CICE). Thanks again!

@apcraig apcraig merged commit 81f1d07 into CICE-Consortium:master May 9, 2019
@phil-blain phil-blain deleted the build-system-robustness branch October 22, 2019 17:25
phil-blain added a commit to phil-blain/Icepack that referenced this pull request Feb 5, 2020
The CFLAG_HOST variable was added in CICE-Consortium#257, and
some Makefile logic was added to define the variable to be blank if
the included Macros file does not define it.

However, the Make syntax is wrong, as 'ifndef' takes a variable name and
not
a reference to a variable [1], so

    ifndef $(CFLAGS_HOST)

should have been written

    ifndef CFLAGS_HOST

The effect of this error is to invert the logic of the check (!) since
if CFLAGS_HOST is defined, Make will check if a variable with name equal
to whatever CFLAGS_HOST is defined to be exists, which will most probably
be false, and the conditional will then evaluate to true and CFLAG_HOSTS
will be redefined to be blank, defeating the whole purpose of the flag.

Since there's no harm in Make referencing and empty variable, fix this
by just removing the conditional check.

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html
phil-blain added a commit to phil-blain/Icepack that referenced this pull request Feb 5, 2020
The CFLAG_HOST variable was added in CICE-Consortium#257, and
some Makefile logic was added to define the variable to be blank if
the included Macros file does not define it.

However, the Make syntax is wrong, as 'ifndef' takes a variable name and
not
a reference to a variable [1], so

    ifndef $(CFLAGS_HOST)

should have been written

    ifndef CFLAGS_HOST

The effect of this error is to invert the logic of the check (!) since
if CFLAGS_HOST is defined, Make will check if a variable with name equal
to whatever CFLAGS_HOST is defined to be exists, which will most probably
be false, and the conditional will then evaluate to true and CFLAG_HOSTS
will be redefined to be blank, defeating the whole purpose of the flag.

Since there's no harm in Make referencing and empty variable, fix this
by just removing the conditional check.

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html
phil-blain added a commit to phil-blain/Icepack that referenced this pull request Feb 6, 2020
The CFLAG_HOST variable was added in CICE-Consortium#257, and
some Makefile logic was added to define the variable to be blank if
the included Macros file does not define it.

However, the Make syntax is wrong, as 'ifndef' takes a variable name and
not
a reference to a variable [1], so

    ifndef $(CFLAGS_HOST)

should have been written

    ifndef CFLAGS_HOST

The effect of this error is to invert the logic of the check (!) since
if CFLAGS_HOST is defined, Make will check if a variable with name equal
to whatever CFLAGS_HOST is defined to be exists, which will most probably
be false, and the conditional will then evaluate to true and CFLAG_HOSTS
will be redefined to be blank, defeating the whole purpose of the flag.

Since there's no harm in Make referencing and empty variable, fix this
by just removing the conditional check.

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html
phil-blain added a commit to phil-blain/Icepack that referenced this pull request Feb 7, 2020
The CFLAG_HOST variable was added in CICE-Consortium#257, and
some Makefile logic was added to define the variable to be blank if
the included Macros file does not define it.

However, the Make syntax is wrong, as 'ifndef' takes a variable name and
not
a reference to a variable [1], so

    ifndef $(CFLAGS_HOST)

should have been written

    ifndef CFLAGS_HOST

The effect of this error is to invert the logic of the check (!) since
if CFLAGS_HOST is defined, Make will check if a variable with name equal
to whatever CFLAGS_HOST is defined to be exists, which will most probably
be false, and the conditional will then evaluate to true and CFLAG_HOSTS
will be redefined to be blank, defeating the whole purpose of the flag.

Since there's no harm in Make referencing and empty variable, fix this
by just removing the conditional check.

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html
apcraig pushed a commit that referenced this pull request Feb 14, 2020
#296)

* machines: add conda environment for macOS and Linux

Closes #294

* Makefile: remove uneeded (and faulty) logic around CFLAGS_HOST

The CFLAG_HOST variable was added in #257, and
some Makefile logic was added to define the variable to be blank if
the included Macros file does not define it.

However, the Make syntax is wrong, as 'ifndef' takes a variable name and
not
a reference to a variable [1], so

    ifndef $(CFLAGS_HOST)

should have been written

    ifndef CFLAGS_HOST

The effect of this error is to invert the logic of the check (!) since
if CFLAGS_HOST is defined, Make will check if a variable with name equal
to whatever CFLAGS_HOST is defined to be exists, which will most probably
be false, and the conditional will then evaluate to true and CFLAG_HOSTS
will be redefined to be blank, defeating the whole purpose of the flag.

Since there's no harm in Make referencing and empty variable, fix this
by just removing the conditional check.

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html

* doc: add documentation for conda environment

* machines: fix Macros file for cheyenne and testmachine

These two files used outdated environment variables for
threading ("compile_threaded") and PIO ("IO_TYPE").

Replace these with the correct variables, ICE_THREADED and
ICE_IOTYPE.

Mirrors CICE-Consortium/CICE#396

* scripts: implement '-nomodules' logic for env files

Some env files wrap the calls for loading the build and run environment
in a conditional based on the argument `-nomodules`, so that
scripts that source the env file just to get the variables defined in it
do not run slower because they have to load the environment.

This logic was copied from CICE but was not implemented in the Icepack
scripts icepack.setup and setup_run_dirs.csh.

Add the argument '-nomodules' so that the logic becomes effective.

* doc: conda: add note about csh/tcsh

On Ubuntu and its derivatives, the csh shell at
/bin/csh, which is used in the shebangs of the Icepack scripts,
is not compatible with conda.

Mention that in the documentation and explain how to install tcsh as an
alternative, and configure the system such that /bin/csh points to
/bin/tcsh.
lettie-roach pushed a commit to lettie-roach/Icepack that referenced this pull request Oct 18, 2022
* Update ice_dyn_eap.F90

* merge in hh eap_efficient branches

* fix declaration of variables for stress_rdg interpolate feature

* update icepack and cleanup log file

* Update ice_dyn_eap.F90

* merge in hh eap_efficient branches

* fix declaration of variables for stress_rdg interpolate feature

* update icepack and cleanup log file

* backup icepack

* remove CAM_ICE and BARRIERS from cice.settings
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