Build system improvements #354

apcraig · 2019-08-16T01:03:53Z

PR checklist

Based on #322. This PR is branched off the current master with changes pulled from @phil-blain build-system-improvements branch. Most changes came from @phil-blain. Due to conflicts with master documentation, a new branch was the easiest way to incorporate the latest changes cleanly.

Includes new targets (db_files, db_flags, targets, clean, makdep, depends, etc) in the Makefile and ability to access them directly via an optional argument passed to cice.build.
makdep is built by the Makefile instead of in the cice.build script.
clean up of include paths
updated documentation, this also addresses cice.build "reuse executable" feature #283.

Documentation changes can be viewed here
more about cice build
build scripts

* cice.make can be used to easily call make with the different targets present in the Makefile (db_files, db_flags, clean), or to pass different flags to make * makdep is listed as a prerequisite of the *.d files, such that we don't need to call make makdep separately in cice.build

The -I flag is used by Fortran compilers to locate files included via the "#include" preprocessor directive or the "include" statement, or modules referenced via the "use" statement. In our codebase we only use modules, and all modules files (*.mod) are put in the ${ICE_OBJDIR} directory, where the compilation takes place. So there is no need for all source directories (contained in the variable $(INCS), prefixed with -I) to be added to the build rules. This shortens the length of the displayed compilation lines.

…g/cice into build-system-improvements2

apcraig · 2019-08-16T01:07:02Z

This PR is largely ready to merge. I will run some final manual tests to confirm errors are trapped (I did this earlier) and I need to make a few minor changes to documentation. This is built on and supercedes #322 and I will close that PR. I wanted to create a PR to push to #322 but that was going to be more complicated due to the need to update to master to get the latest documentation.

apcraig · 2019-08-22T15:43:51Z

This is ready to merge. @phil-blain, if you get a chance to have a look, let me know. Same for @eclare108213. Thanks.

phil-blain · 2019-08-22T19:26:16Z

I will do that as soon as I get back to work on Monday. I’m out of the office this week, sorry for the delay.

apcraig · 2019-08-22T22:32:47Z

No problem, thanks @phil-blain

eclare108213

There's a spelling error in dg_scripts.rst ('resuse').
Also 'recommendeded' in ug_running.rst.

Can this be removed from cice.build?

# tcraig, this is handled below, is it needed here?
#if (${ICE_CLEANBUILD} == 'true') then
#  echo "cleaning objdir"
#  rm -r -f ${ICE_OBJDIR}
#endif

Otherwise, no complaints from me. Very cool, thanks!

phil-blain

Thanks a lot for working on that Tony, I think the changes look good. I've highlighted a few points below (hope I'm not too picky!). I can do a few of those if you agree, just let me know.

phil-blain · 2019-08-26T17:41:35Z

configuration/scripts/Makefile

@@ -156,6 +170,8 @@ realclean:
 #   are understood & accepted.
 #-------------------------------------------------------------------------------

+depends: $(DEPS)


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.

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?

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.

phil-blain · 2019-08-26T17:41:47Z

configuration/scripts/Makefile

@@ -66,18 +66,28 @@ RM    := rm
 .SUFFIXES:
 .SUFFIXES: .F90 .F .c .o

+.PHONY: all


a single .PHONY: all cice db_files db_flags mostlyclean clean realclean line would also work

OK, good to know. I may do that instead, it's cleaner.

phil-blain · 2019-08-26T17:54:23Z

configuration/scripts/Makefile

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


I think we can take this opportunity to simplify the clean targets. I don't think we need 3. I think that having a 'clean' target that deletes all *.o, *.mod, *.d and the exe is sufficient, plus a 'realclean' target that does the same plus deletes the 'makdep' executable. (this way the 'clean' target does the same thing as a clean build in terms of the CICE code itself)

I prefer the 'rm' command in these targets to use '*.o' instead of '$(OBJS)' (for example) since the latter dumps a lot of text to the command line. (and in clean builds, this is amongst the first text that is outputed)

I think we can remove the *.f and *.f90 from these targets since our build system does not generate any preprocessed sources.

I suggest the following, which is also simpler :

clean: $(RM) -f *.o *.d *.mod $(EXEC) realclean: clean $(RM) -f $(DEPGEN)

I agree. I will refactor a bit.

One other comment, the clean that the CICE code does by default is now using the Makefile clean. I changed that recently (maybe in this PR), not sure why it wasn't doing that before.

phil-blain · 2019-08-26T18:22:07Z

configuration/scripts/cice.build

@@ -79,6 +142,18 @@ ${ICE_SANDBOX}/cicecore/shared
 ${ICE_SANDBOX}/icepack/columnphysics
 EOF

+if (${directmake} == 1) then


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)

Good catch, I am making those changes.

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.

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.

phil-blain · 2019-08-26T18:30:55Z

doc/source/user_guide/ug_running.rst

+
+  cice.build [mostlyclean|clean|realclean]
+
+to write out information about the Makefile setup,::


the comma at the end of the line should be removed

phil-blain · 2019-08-26T18:30:59Z

doc/source/user_guide/ug_running.rst

+
+  cice.build [db_files|db_flags]
+
+and to build the makdep tool or the dependencies,::


I would not mention the 'makdep' or 'depends' target in the doc. Usually people would just run cice.build so I think this could be confusing

I'm not sure I agree 100%. Maybe leave it in for now. I think it could be useful to be able to create the dependencies without actually building the model. Maybe if we need to check whether they are working correctly or for other reasons. It might be reasonable to not document it though. Let me think about that.

phil-blain · 2019-08-26T18:47:20Z

configuration/scripts/cice.build

-  set ciceexe = $1
+if ($#argv == 0) then
+  # continue, standard way to build
+else if ($#argv == 1) then


I think the argument checking is too drastic; it prevents one from using all the features of make (ex. see https://www.gnu.org/software/make/manual/make.html#Instead-of-Execution where some flags that are supported by make are listed). This is one of the reason I passed all arguments in my initial implementation ($*). I suggest that we check if the first argument is '--exe', '-h' or '--help' but apart from that we pass all other arguments to make. This way we can use an arbitrary number of make flags, both when using cice.build or cice.build target

OK, let me see what I can do. I didn't think about passing command line arguments directly to gmake and wanted to keep the parsing simple. But I can see what that might be useful, I will try to refactor. Thanks for the careful review, not too picky at all!

@phil-blain, what do you think the requirements are here? Is it just

cice.build # standard approach
cice.build -h
cice.build target # our makefile targets
cice.build [gmake option] # gmake options, one flag only
cice.build --exe filename

or is there also a requirement for

cice.build [gmake option] [gmake option] ... [target]

and so forth. Should we support gmake arguments with and/or without a target like

gmake --debug --keep-going
gmake --debug cice
gmake --silent --keep-going cice

and others as well? It seems to me if we are going to allow the ability to pass the gmake arguments from cice.build to gmake, it needs to be general, support multiple arguments, and work with a target as well. I will try to do that.

I think I've got it working, we definitely want the most flexibility and I think that was suggested in the review.

Yes, I agree that more flexibility is good, so all of these should work :

cice.build # standard approach
cice.build -h
cice.build target # our makefile targets
cice.build target1 target2 ... # multiple Makefile targets
cice.build [gmake option] [gmake option] [... other options ....] # gmake options
cice.build [gmake option] [gmake option] [[... other options ....] target # gmake options and target
cice.build [gmake option] [gmake option] [[... other options ....] target1 target2 ... # gmake options and multiple targets
cice.build --exe filename

As a side note, with the changes you already made we are now able to call
cice.build ice_dyn_evp.o
to compile a single file. This can be useful sometimes when you are just debugging compilation errors in a single file (This works because all objects file are implicit targets in the Makefile).
Do you think this should be mentioned in the documentation ?

apcraig · 2019-08-27T21:58:34Z

I just checked in some changes based on the reviews and think I have covered the main recommendations.

In the Makefile, I updated the PHONY lines
I cleaned up the different clean options
I refactored the argument parsing in cice.build and it now supports passing arbitrary arguments to gmake
I decided that if the gmake was going to be called with a target, the rest of the features of the cice.build script should not be invoked. I think it makes sense to treat calling cice.build with arguments and a target like it's just calling make directly as much as possible. I think if we try to mix "full featured" cice.build calls direct gmake calls it will get complicated.
I cleaned up the documentation typo.
I updated other parts of the documentation to clarify the implementation and make it consistent with the latest changes. See https://apcraig-cice.readthedocs.io/en/build-system-improvements2/user_guide/ug_running.html#more-about-cice-build

@phil-blain , if you are willing to take another look and review, that would be great. I have tested this on conrad again and it seems to work fine, #eab1b65 https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks

phil-blain

Super! I tried the new script and everything works great. I noted a few minor points below

phil-blain · 2019-08-28T14:22:04Z

configuration/scripts/Makefile

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


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

good catch.

phil-blain · 2019-08-28T14:26:40Z

configuration/scripts/Makefile

@@ -188,7 +179,6 @@ depends: $(DEPS)
 # 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)


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

phil-blain · 2019-08-28T14:29:16Z

configuration/scripts/Makefile

-realclean:
-	$(RM) -f *.f *.f90 *.d *.mod *.o $(DEPS) $(OBJS) $(DEPGEN) $(EXEC)
+realclean: clean
+	$(RM) -f $(DEPGEN)

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


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) $<

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.

phil-blain · 2019-08-28T14:32:07Z

configuration/scripts/cice.build

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


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

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.

I understand.

phil-blain · 2019-08-28T15:18:15Z

doc/source/user_guide/ug_running.rst

+default, this variable is true which means each invokation of **cice.build** will
+automatically clean the prior build.  If incremental builds are desired to save
+time during development, the ``ICE_CLEANBUILD`` setting in **cice.settings** should
+be modified.


Maybe we should also mention the 'buildincremental' option to cice.setup here ? (just an idea)

In some ways, I prefer to not encourage use of the incremental builds. Also, I think we have documented that feature elsewhere and prefer not to duplicate documentation if we can help it. It makes the documentation harder to maintain and makes the document bigger (but not necessarily clearer). I think we should always be evaluating whether the organization of the documentation is "best" and if not, to reorganize to make it easier to find stuff and clearer. I think we want to avoid duplication if possible.

phil-blain · 2019-08-28T15:21:03Z

doc/source/user_guide/ug_running.rst

+Makefile and a machine specific Macros file in the case directory.  **cice.build**
+is a wrapper for a call to make that includes several other features.  
+
+CICE is built as follows.  First, the makdep binary is created by compiling a small


I think C should be capitalized. Also, there is actually one dependency file per object file, so I would write this part as :

First, the makdep binary is created by compiling a small C program. The makdep binary is then run and dependency files are created. The dependency files are included into the Makefile automatically. As a result, make dependencies do not need to be explicitly defined nor generated by the user.

Thanks, I will fix this as you suggest. Running a quick test now with the changes and will push soon.

apcraig · 2019-08-28T16:51:55Z

OK, I just pushed a couple more changes based on the latest reviews. If @phil-blain agrees, I think we can merge the PR. If other changes are suggested, happy to keep iterating. thanks!

phil-blain and others added 7 commits June 11, 2019 09:05

Update db_files and db_flags rules in Makefile

eb05a26

Update documentation

a73ac1f

add cice.build argument capabilities

0559c8e

Merge branch 'build-system-improvements' of https://github.com/apcrai…

6858fc5

…g/cice into build-system-improvements2

update documentation

1e6e144

apcraig requested review from eclare108213, mattdturner and phil-blain August 16, 2019 01:03

apcraig self-assigned this Aug 16, 2019

apcraig mentioned this pull request Aug 16, 2019

Build system improvements #322

Closed

apcraig added Priority: Medium Scripts Type: Feature labels Aug 16, 2019

apcraig added 2 commits August 16, 2019 01:12

update documentation

fe243ff

update documentation

818e6b6

eclare108213 approved these changes Aug 22, 2019

View reviewed changes

cleanup of documentation and implementation

eab1b65

phil-blain suggested changes Aug 26, 2019

View reviewed changes

apcraig added 5 commits August 27, 2019 20:43

update implementation based on reviews

ba03da0

update documentation

cb33be4

update documentation

f9c6eff

update documentation

aa25e7f

update documentation

d412599

phil-blain reviewed Aug 28, 2019

View reviewed changes

update Makefile and documentation

ea27839

phil-blain approved these changes Aug 28, 2019

View reviewed changes

apcraig merged commit 3e0ec92 into CICE-Consortium:master Aug 28, 2019

phil-blain mentioned this pull request Aug 29, 2019

cice.build "reuse executable" feature #283

Closed

phil-blain mentioned this pull request Sep 4, 2019

cice.build does not write stderror to ${ICE_BLDLOG_FILE} phil-blain/CICE#2

Closed

apcraig deleted the build-system-improvements2 branch August 17, 2022 20:59


		cice.build [mostlyclean\|clean\|realclean]

		to write out information about the Makefile setup,::


		cice.build [db_files\|db_flags]

		and to build the makdep tool or the dependencies,::

Build system improvements #354

Build system improvements #354

Conversation

apcraig commented Aug 16, 2019

PR checklist

apcraig commented Aug 16, 2019

apcraig commented Aug 22, 2019

phil-blain commented Aug 22, 2019

apcraig commented Aug 22, 2019

eclare108213 left a comment

Choose a reason for hiding this comment

phil-blain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-blain Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

apcraig commented Aug 27, 2019

phil-blain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-blain Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apcraig commented Aug 28, 2019

phil-blain Aug 27, 2019 •

edited

Loading

phil-blain Aug 28, 2019 •

edited

Loading