Skip to content

Move makdep compilation to Makefile#307

Merged
apcraig merged 3 commits intoCICE-Consortium:masterfrom
phil-blain:build-system-robustness
Apr 30, 2019
Merged

Move makdep compilation to Makefile#307
apcraig merged 3 commits intoCICE-Consortium:masterfrom
phil-blain:build-system-robustness

Conversation

@phil-blain
Copy link
Copy Markdown
Member

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

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/.

* makdep compiled from a makdep rule in the Makefile
* Add CFLAG_HOST macro
* makdep is only compiled if needed
* This addresses CICE-Consortium#306
Copy link
Copy Markdown
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.

This looks good. I will test on a few other systems before merging. Will try to do that this week.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 phil-blain force-pushed the build-system-robustness branch from 71e7bb2 to 6d990a7 Compare April 30, 2019 12:47
@phil-blain
Copy link
Copy Markdown
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
Copy Markdown
Member Author

Copy link
Copy Markdown
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.

Excellent, thanks for this. Looks great.

@apcraig apcraig merged commit b0063cb into CICE-Consortium:master Apr 30, 2019
@phil-blain phil-blain deleted the build-system-robustness branch August 30, 2019 17:27
phil-blain added a commit to phil-blain/CICE that referenced this pull request Jan 14, 2020
The CFLAG_HOST variable was added in CICE-Consortium#307, 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 Jan 24, 2020
* machines: add env and Macro files for conda on Linux and macOS

* machines: add environment.yml conda specification

* doc: add section on laptop computers

Uses the conda environment.

* doc: recommend not to touch the 'cice' conda environment

* machines: add CFLAGS_HOST to conda_macos for recent macOS

Recent versions of Xcode and macOS do not install the system C header files
to /usr/include, but keep them in the MacOSX SDK.
Thus we need to explicitely pass their location to the compiler.

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

The CFLAG_HOST variable was added in #307, 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: use curl instead of wget on macOS

wget is not installed by default

* machines: conda: invoke 'conda info --base' using $CONDA_EXE

This variable is always defined to the path to the conda executable
when the login shell is properly initialized.

Calling `conda info --base` works if the shell initialization procedure
puts the conda executable in the $PATH, which is the case for Bash and Zsh
but not for tcsh.

* doc: conda: add initialization instructions for alternative login shells

The Miniconda installer only initializes Bash on macOS and Linux,
so additional steps needs to be taken if your login shell is different.

Add instructions for tcsh, zsh, fish and xonsh.

* machines: conda: add error checking to env files

Let's check if the conda executable is found and if the cice
conda environment exists.

* doc: separate Miniconda installation and conda initialization

- Move the documentation for the conda configuration under "Porting"
- Move the instructions for initializing the user's shell for use with conda
  to a speparate section
- Add a new section for manually initializing the user's shell if they do not
  want to modify their startup files
- Emphasize which steps are "first-time setup" steps
- Add details about how the conda environment is activated during build/run

* doc: conda: mention symlinks and creating a complete port
phil-blain added a commit to phil-blain/CICE that referenced this pull request Apr 15, 2020
…CE-Consortium#393)

* machines: add env and Macro files for conda on Linux and macOS

* machines: add environment.yml conda specification

* doc: add section on laptop computers

Uses the conda environment.

* doc: recommend not to touch the 'cice' conda environment

* machines: add CFLAGS_HOST to conda_macos for recent macOS

Recent versions of Xcode and macOS do not install the system C header files
to /usr/include, but keep them in the MacOSX SDK.
Thus we need to explicitely pass their location to the compiler.

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

The CFLAG_HOST variable was added in CICE-Consortium#307, 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: use curl instead of wget on macOS

wget is not installed by default

* machines: conda: invoke 'conda info --base' using $CONDA_EXE

This variable is always defined to the path to the conda executable
when the login shell is properly initialized.

Calling `conda info --base` works if the shell initialization procedure
puts the conda executable in the $PATH, which is the case for Bash and Zsh
but not for tcsh.

* doc: conda: add initialization instructions for alternative login shells

The Miniconda installer only initializes Bash on macOS and Linux,
so additional steps needs to be taken if your login shell is different.

Add instructions for tcsh, zsh, fish and xonsh.

* machines: conda: add error checking to env files

Let's check if the conda executable is found and if the cice
conda environment exists.

* doc: separate Miniconda installation and conda initialization

- Move the documentation for the conda configuration under "Porting"
- Move the instructions for initializing the user's shell for use with conda
  to a speparate section
- Add a new section for manually initializing the user's shell if they do not
  want to modify their startup files
- Emphasize which steps are "first-time setup" steps
- Add details about how the conda environment is activated during build/run

* doc: conda: mention symlinks and creating a complete port
phil-blain added a commit to phil-blain/CICE that referenced this pull request May 29, 2020
…CE-Consortium#393)

* machines: add env and Macro files for conda on Linux and macOS

* machines: add environment.yml conda specification

* doc: add section on laptop computers

Uses the conda environment.

* doc: recommend not to touch the 'cice' conda environment

* machines: add CFLAGS_HOST to conda_macos for recent macOS

Recent versions of Xcode and macOS do not install the system C header files
to /usr/include, but keep them in the MacOSX SDK.
Thus we need to explicitely pass their location to the compiler.

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

The CFLAG_HOST variable was added in CICE-Consortium#307, 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: use curl instead of wget on macOS

wget is not installed by default

* machines: conda: invoke 'conda info --base' using $CONDA_EXE

This variable is always defined to the path to the conda executable
when the login shell is properly initialized.

Calling `conda info --base` works if the shell initialization procedure
puts the conda executable in the $PATH, which is the case for Bash and Zsh
but not for tcsh.

* doc: conda: add initialization instructions for alternative login shells

The Miniconda installer only initializes Bash on macOS and Linux,
so additional steps needs to be taken if your login shell is different.

Add instructions for tcsh, zsh, fish and xonsh.

* machines: conda: add error checking to env files

Let's check if the conda executable is found and if the cice
conda environment exists.

* doc: separate Miniconda installation and conda initialization

- Move the documentation for the conda configuration under "Porting"
- Move the instructions for initializing the user's shell for use with conda
  to a speparate section
- Add a new section for manually initializing the user's shell if they do not
  want to modify their startup files
- Emphasize which steps are "first-time setup" steps
- Add details about how the conda environment is activated during build/run

* doc: conda: mention symlinks and creating a complete port
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