Skip to content

Build system improvements #354

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

Merged
apcraig merged 16 commits into from
Aug 28, 2019
Merged

Build system improvements #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b998b83
Add cice.make script, list makdep as prerequisite
phil-blain Apr 25, 2019
eb05a26
Update db_files and db_flags rules in Makefile
phil-blain Apr 25, 2019
01f3873
Remove $(INCS) variable from build rules
phil-blain Apr 25, 2019
a73ac1f
Update documentation
phil-blain Jun 11, 2019
0559c8e
add cice.build argument capabilities
apcraig Aug 15, 2019
6858fc5
Merge branch 'build-system-improvements' of https://github.com/apcrai…
apcraig Aug 15, 2019
1e6e144
update documentation
apcraig Aug 15, 2019
fe243ff
update documentation
apcraig Aug 16, 2019
818e6b6
update documentation
apcraig Aug 16, 2019
eab1b65
cleanup of documentation and implementation
apcraig Aug 23, 2019
ba03da0
update implementation based on reviews
apcraig Aug 27, 2019
cb33be4
update documentation
apcraig Aug 27, 2019
f9c6eff
update documentation
apcraig Aug 27, 2019
aa25e7f
update documentation
apcraig Aug 27, 2019
d412599
update documentation
apcraig Aug 27, 2019
ea27839
update Makefile and documentation
apcraig Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cice.setup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ echo "${testname_base}"
cd ${testname_base}
source ./cice.settings
set ciceexe = "../ciceexe.\${ICE_COMPILER}.\${ICE_COMMDIR}.\${ICE_BLDDEBUG}.\${ICE_THREADED}.\${ICE_IOTYPE}"
./cice.build \${ciceexe}
./cice.build --exe \${ciceexe}
if !(-e \${ciceexe}) cp -p \${ICE_RUNDIR}/cice \${ciceexe}
EOF
end
Expand Down
79 changes: 51 additions & 28 deletions configuration/scripts/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
# SRCS=<files> ~ list of src files, default is all .c .F .F90 files in VPATH
# VPFILE=<file> ~ file with list of dirs, used to create VPATH
# SRCFILE=<file> ~ file with list of src files, used to create SRCS
# DEPGEN=<exec> ~ dependency generator utility, default is makdep
#
# <macro defns> ~ any macro definitions found in this file or the included
# MACFILE will be over-riden by cmd-line macro definitions
Expand All @@ -31,7 +30,9 @@ VPFILE := NONE
VPATH := .
SRCFILE := NONE
SRCS := NONE
DEPGEN := ./makdep # an externally provided dependency generator

# dependency generator
DEPGEN := ./makdep

ifneq ($(VPATH),.)
# this variable was specified on cmd line or in an env var
Expand All @@ -58,26 +59,33 @@ endif
OBJS := $(addsuffix .o, $(sort $(basename $(notdir $(SRCS)))))
DEPS := $(addsuffix .d, $(sort $(basename $(notdir $(SRCS)))))
INCS := $(patsubst %,-I%, $(VPATH) )
OBJS_DEPGEN := $(addprefix $(ICE_CASEDIR)/,$(addsuffix .c, $(notdir $(DEPGEN))))
MODDIR:= -I.
RM := rm

.SUFFIXES:
.SUFFIXES: .F90 .F .c .o

.PHONY: all cice targets target db_files db_flags clean realclean
all: $(EXEC)

cice: $(EXEC)

#-------------------------------------------------------------------------------
# include the file that provides macro definitions required by build rules
# note: the MACFILE may not be needed for certain goals
#-------------------------------------------------------------------------------

ifneq ($(MAKECMDGOALS), db_files)
-include $(MACFILE)
endif
-include $(MACFILE)

#-------------------------------------------------------------------------------
# echo file names, paths, compile flags, etc. used during build
# echo supported targets, file names, paths, compile flags, etc. used during build
#-------------------------------------------------------------------------------

targets:
@echo " "
@echo "Supported Makefile Targets are: cice, makdep, depends, clean, realclean, targets, db_files, db_flags"
target: targets

@phil-blain phil-blain Aug 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the new target 'target' should be added to the .PHONY line

@apcraig apcraig Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch.


db_files:
@echo " "
@echo "* EXEC := $(EXEC)"
Expand All @@ -89,12 +97,22 @@ db_files:
@echo "* SRCS := $(SRCS)"
@echo "* OBJS := $(OBJS)"
@echo "* DEPS := $(DEPS)"
@echo "* ULIBS := $(ULIBS)"
@echo "* SLIBS := $(SLIBS)"
@echo "* INCLDIR := $(INCLDIR)"
@echo "* OBJS_DEPGEN := $(OBJS_DEPGEN)"
db_flags:
@echo " "
@echo "* cpp := $(CPP) $(CPPFLAGS) $(CPPDEFS) $(INCS) $(INCLDIR)"
@echo "* cc := $(CC) -c $(CFLAGS) $(INCS) $(INCLDIR)"
@echo "* .F.o := $(FC) -c $(FFLAGS) $(FIXEDFLAGS) $(INCS) $(INCLDIR)"
@echo "* .F90.o := $(FC) -c $(FFLAGS) $(FREEFLAGS) $(INCS) $(INCLDIR)"
@echo "* $(DEPGEN) := $(SCC) $(CFLAGS_HOST)"
@echo "* %.d : %.c := $(DEPGEN) $(INCS)"
@echo "* %.d : %.F := $(DEPGEN) $(INCS)"
@echo "* %.d : %.F90 := $(DEPGEN) $(INCS)"
@echo "* %.d : %.H := $(DEPGEN) $(INCS)"
@echo "* cpp := $(CPP) $(CPPFLAGS) $(CPPDEFS) $(INCLDIR)"
@echo "* .c.o := $(CC) $(CFLAGS) $(CPPDEFS) $(INCLDIR)"
@echo "* .F.o := $(FC) -c $(FFLAGS) $(FIXEDFLAGS) $(CPPDEFS) $(INCLDIR)"
@echo "* .F90.o := $(FC) -c $(FFLAGS) $(FREEFLAGS) $(CPPDEFS) $(MODDIR) $(INCLDIR)"
@echo "* $(notdir $(EXEC)) := $(LD) $(LDFLAGS) $(ULIBS) $(SLIBS)"

#-------------------------------------------------------------------------------
# build rule for makdep: MACFILE, cmd-line, or env vars must provide
Expand All @@ -105,7 +123,8 @@ ifndef $(CFLAGS_HOST)
CFLAGS_HOST :=
endif

$(DEPGEN): $(ICE_CASEDIR)/makdep.c
$(DEPGEN): $(OBJS_DEPGEN)
@ echo "Building makdep"
$(SCC) -o $@ $(CFLAGS_HOST) $<

#-------------------------------------------------------------------------------
Expand All @@ -116,58 +135,62 @@ $(EXEC): $(OBJS)
$(LD) -o $(EXEC) $(LDFLAGS) $(OBJS) $(ULIBS) $(SLIBS)

.c.o:
$(CC) $(CFLAGS) $(CPPDEFS) $(INCS) $(INCLDIR) $<
$(CC) $(CFLAGS) $(CPPDEFS) $(INCLDIR) $<

.F.o:
$(FC) -c $(FFLAGS) $(FIXEDFLAGS) $(CPPDEFS) $(INCS) $(INCLDIR) $<
$(FC) -c $(FFLAGS) $(FIXEDFLAGS) $(CPPDEFS) $(INCLDIR) $<

.F90.o:
$(FC) -c $(FFLAGS) $(FREEFLAGS) $(CPPDEFS) $(INCS) $(INCLDIR) $<

mostlyclean:
$(RM) -f *.f *.f90
$(FC) -c $(FFLAGS) $(FREEFLAGS) $(CPPDEFS) $(MODDIR) $(INCLDIR) $<

clean:
$(RM) -f *.f *.f90 *.d *.mod *.o $(EXEC)
# $(RM) -f *.f *.f90 *.d *.$(MOD_SUFFIX) $(OBJS)
$(RM) -f *.o *.d *.mod $(EXEC)

realclean:
$(RM) -f *.f *.f90 *.d *.$(MOD_SUFFIX) $(OBJS) $(EXEC)
realclean: clean
$(RM) -f $(DEPGEN)

#-------------------------------------------------------------------------------
# Build & include dependency files

@phil-blain phil-blain Aug 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could also take this opportunity to clean up what is uneeded here : all our files are *.F90 or *.c so the dependency generation for *.F and *.H can be removed.
Same for the rule

.F.o:
	$(FC) -c $(FFLAGS) $(FIXEDFLAGS) $(CPPDEFS) $(INCLDIR) $<

@apcraig apcraig Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's OK to leave the other make rules in the Makefile for now. It could be that future development will need them. I don't think it hurts to have them there. I don't think folks actually look at the Makefile all that much. But I could be convinced otherwise. For now, I think I'll leave them as is.

#-------------------------------------------------------------------------------
# ASSUMPTIONS:
# o an externally provided dependency generator, $(DEPGEN), is available,
# o the dependency generator, $(DEPGEN), can be built,
# its cmd line syntax is compatible with the build rules below. Eg, for
# each .o file, there is a corresponding .d (dependency) file, and both
# will be dependent on the same src file, eg. foo.o foo.d : foo.F90
# Also, the dependancy genorator's capabilities, limitations, and assumptions
# are understood & accepted.
#-------------------------------------------------------------------------------

%.d : %.c
depends: $(DEPS)

@phil-blain phil-blain Aug 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure the 'depends' target is needed... it's only use would be to manually recreate the dependencies by calling cice.build depends, right ? I don't see a use case for that; either the user uses incremental builds or clean builds; in both cases the makefile generates the dependencies before starting the compilation. I think that having a depends target can create some confusion.

@apcraig apcraig Aug 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you're right that we may not need depends. I don't think it hurts to have it. Maybe someone wants to just generate the depends without building the rest of the code?

@phil-blain phil-blain Aug 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are right that it does not hurt to have it. Although in that case I would mention somewhere in the documentation that the dependencies are usually automatically generated.


%.d : %.c $(DEPGEN)
@ echo "Building dependency for $@"
@ $(DEPGEN) -f $(INCS) $< | head -3 > $@
%.d : %.F
%.d : %.F $(DEPGEN)
@ echo "Building dependency for $@"
@ $(DEPGEN) -f $(INCS) $< > $@
%.d : %.F90
%.d : %.F90 $(DEPGEN)
@ echo "Building dependency for $@"
@ $(DEPGEN) -f $(INCS) $< > $@
%.d : %.H
%.d : %.H $(DEPGEN)
@ echo "Building dependency for $@"
@ $(DEPGEN) -f $(INCS) $< > $@

# the if-tests prevent DEPS files from being created when they're not needed
ifneq ($(MAKECMDGOALS), db_files)
ifneq ($(MAKECMDGOALS), db_flags)
ifneq ($(MAKECMDGOALS), mostlyclean)
ifneq ($(MAKECMDGOALS), clean)
ifneq ($(MAKECMDGOALS), realclean)
ifneq ($(MAKECMDGOALS), targets)

@phil-blain phil-blain Aug 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the new target 'target' should also have it's line here : ifneq ($(MAKECMDGOALS), target)

@apcraig apcraig Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch

ifneq ($(MAKECMDGOALS), target)
ifneq ($(MAKECMDGOALS), makdep)
ifneq ($(MAKECMDGOALS), depends)
-include $(DEPS)
endif
endif
endif
endif
endif
endif
endif
endif
128 changes: 98 additions & 30 deletions configuration/scripts/cice.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,93 @@
# If the cice binary is passed as an argument and the file exists,
# copy it into the run directory and don't build the model.

set dohelp = 0
set directmake = 0
set target = "UnDEFineD"
set ciceexe = "UnDEFineD"
if ($#argv == 1) then
set ciceexe = $1
echo "${0}: ciceexe = ${ciceexe}"
if (-e ${ciceexe}) then

source ./cice.settings
source ${ICE_CASEDIR}/env.${ICE_MACHCOMP} -nomodules || exit 2
if !(-d ${ICE_RUNDIR}) mkdir -p ${ICE_RUNDIR}
cp -p ${ciceexe} ${ICE_RUNDIR}/cice

echo "`date` ${0}:${ICE_CASENAME} build copied ${ciceexe}" >> ${ICE_CASEDIR}/README.case
if ( ${ICE_TEST} != ${ICE_SPVAL} ) then
echo "#---" >! ${ICE_CASEDIR}/test_output
echo "COPY ${ICE_TESTNAME} build" >> ${ICE_CASEDIR}/test_output
echo "PEND ${ICE_TESTNAME} run" >> ${ICE_CASEDIR}/test_output
if ($#argv == 0) then
# continue, standard way to build
else
# -h, --help
if ("$1" == "-h" || "$1" == "--help") then
set dohelp = 1

# --exe
else if ("$1" == "--exe") then
if ($#argv > 2) then
echo "${0}: ERROR: --exe ciceexe without other arguments is required"
exit -99
endif
set ciceexe = $2
echo "${0}: ciceexe = ${ciceexe}"
if (-e ${ciceexe}) then

source ./cice.settings
source ${ICE_CASEDIR}/env.${ICE_MACHCOMP} -nomodules || exit 2
if !(-d ${ICE_RUNDIR}) mkdir -p ${ICE_RUNDIR}
cp -p ${ciceexe} ${ICE_RUNDIR}/cice

echo "`date` ${0}:${ICE_CASENAME} build copied ${ciceexe}" >> ${ICE_CASEDIR}/README.case
if ( ${ICE_TEST} != ${ICE_SPVAL} ) then
echo "#---" >! ${ICE_CASEDIR}/test_output
echo "COPY ${ICE_TESTNAME} build" >> ${ICE_CASEDIR}/test_output
echo "PEND ${ICE_TESTNAME} run" >> ${ICE_CASEDIR}/test_output
endif

exit 0
endif

exit 0

# direct make with target
else
set directmake = 1
set target = "$*"
endif
endif

if (${dohelp} == 1) then
cat << EOF1

NAME
cice.build [-h,--help] [make arguments] [target]

SYNOPSIS
-h || --help
[make arguments] [target]

DESCRIPTION
--help, -h : help
[make arguments] [target] : specify make arguments and target

EXAMPLES
cice.build
will build the model using the standard approach with all the bells and whistles.
This is recommended.
cice.build --help
will show cice.build help
cice.build [target]
will call gmake directly and make the target
cice.build clean
is an example of a targeted build
cice.build targets
is an example of a targeted build that shows the valid Makefile targets
cice.build --version
will call make directly and pass --version as an argument to make
cice.build [make arguments] [target]
will call make directly and pass arguments and/or a target to make

ADDITIONAL INFORMATION
The argument implementation supports -h or --help as a first argument. Otherwise,
it passes all other arguments directly to make and calls it directly. In this
mode, most of the cice.build script features are by-passed. The recommended
way to run the script is without arguments.

SEE ALSO
User Documentation at https://github.com/cice-consortium/cice/

@phil-blain phil-blain Aug 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to link to read-the-docs here instead ?

@apcraig apcraig Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is what version of readthedocs? Almost certainly whatever version we might link in the help output is incorrect. Master documentation is only correct for the current master. For released versions, would we need to manually change the link for each release? This is a case where it's easy for things to get out of sync. While pointing to the general cice repo isn't particularly useful either, at least I feel like it's never incorrect.

@phil-blain phil-blain Aug 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand.


EOF1
exit -99
endif

#====================================

source ./cice.settings
Expand All @@ -47,10 +111,7 @@ set stamp = `date '+%y%m%d-%H%M%S'`
set ICE_BLDLOG_FILE = "cice.bldlog.${stamp}"
set quiet = ${ICE_QUIETMODE}

if (${ICE_CLEANBUILD} == 'true') then
echo "cleaning objdir"
rm -r -f ${ICE_OBJDIR}
endif
if !(-d ${ICE_RUNDIR}) mkdir -p ${ICE_RUNDIR}
if !(-d ${ICE_OBJDIR}) mkdir -p ${ICE_OBJDIR}
cd ${ICE_OBJDIR}

Expand Down Expand Up @@ -79,6 +140,22 @@ ${ICE_SANDBOX}/cicecore/shared
${ICE_SANDBOX}/icepack/columnphysics
EOF

if !($?ICE_MACHINE_BLDTHRDS) then
set ICE_MACHINE_BLDTHRDS = 1
endif

if (${directmake} == 1) then

@phil-blain phil-blain Aug 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • I would actually put this block just before line if (${ICE_CLEANBUILD} == 'true') then. This way ICE_* variables are always shown, and ICE_MACHINE_BLDTHRDS is defined (the way it is now, the '-j' flag will be passed to Make in all cases, even if the ICE_MACHINE_BLDTHRDS variable is not defined, and Make will use all threads available on the machine (I've read this can impact performance...)
  • Also, I think that in the 'directmake ' case we should still write to the build log file with ... |& tee ${ICE_BLDLOG_FILE}. This way we can also trap circular dependencies in that case. (Maybe we should also respect the ICE_QUIETMODE variable, though I personally don't care for that one)

@apcraig apcraig Aug 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I am making those changes.

@apcraig apcraig Aug 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the second point, I think we decided for a direct make to avoid all the fluff. I think writing the make output to a bldlog file is part of that. I guess I view the circular dependencies as part of the fluff you get if you use the scripts the standard way. Also, the quietmode should not generally be used except by travis. For whatever reason, if we don't use the quiet mode with travis, it creates problems. it either concatenates the raw log file or it makes it very hard to find the errors. this was added just for travis and serves a purpose.

@phil-blain phil-blain Aug 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, I guess I viewed the 'fluff' as only the README.case file. But I don't feel strongly about that. So either way is fine.
Regarding quietmode, I didn't know the history, I thought it predated our Git/GitHub migration.

echo "make ${target}"
${ICE_MACHINE_MAKE} -j ${ICE_MACHINE_BLDTHRDS} VPFILE=Filepath EXEC=${ICE_RUNDIR}/cice \
-f ${ICE_CASEDIR}/Makefile MACFILE=${ICE_CASEDIR}/Macros.${ICE_MACHCOMP} ${target}
set bldstat = ${status}
if (${bldstat} != 0) then
echo "${0}: targeted make FAILED"
exit -99
endif
exit 0
endif

echo " "
echo ICE_GRID = ${ICE_GRID}
echo ICE_NTASK = ${ICE_NTASKS}
Expand All @@ -88,16 +165,7 @@ echo "Filepath = "
cat ${ICE_OBJDIR}/Filepath
echo " "

echo "building makdep"
${ICE_MACHINE_MAKE} \
-f ${ICE_CASEDIR}/Makefile MACFILE=${ICE_CASEDIR}/Macros.${ICE_MACHCOMP} makdep || exit 2

echo "building cice > ${ICE_OBJDIR}/${ICE_BLDLOG_FILE}"

if !(-d ${ICE_RUNDIR}) mkdir -p ${ICE_RUNDIR}
if !($?ICE_MACHINE_BLDTHRDS) then
set ICE_MACHINE_BLDTHRDS = 1
endif
if (-e ${ICE_BLDLOG_FILE}) rm ${ICE_BLDLOG_FILE}

if (${ICE_CLEANBUILD} == 'true') then
Expand Down
7 changes: 7 additions & 0 deletions doc/source/developer_guide/dg_scripts.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ The machine is built by the cice.build script which invokes Make.
There is a special trap for circular dependencies in the cice.build script to
highlight this error when it occurs.

The **cice.build** script has some additional features including the ability to
pass a Makefile target. This is documented in :ref:`cicebuild`. In addition, there
is a hidden feature in the **cice.build** script that allows for reuse of
executables. This is used by the test suites to significantly reduce cost of
building the model. It is invoked with the ``--exe`` argument to **cice.build**
and should not be invoked by users interactively.

.. _dev_machines:

Machines
Expand Down
Loading