Add checks for circular dependencies in cice.build

[phil-blain]

PR checklist

• Short (1 sentence) summary of your PR:
  Add checks for circular dependencies in cice.build
• Developer(s):
  Philippe Blain, Tony Craig
• Suggest PR reviewers from list in the column to the right. @apcraig @eclare108213
• Please copy the PR test results link or provide a summary of testing completed below.
  No tests - no changes to code
• How much do the PR code changes differ from the unmodified code?
  • bit for bit
  • different at roundoff level
  • more substantial
• Does this PR create or have dependencies on Icepack or any other models?
  • Yes
  • No
• Does this PR add any new test cases?
  • Yes
  • No
• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes, if we add a 'system requirements' section
    • No
• Please provide any additional information or relevant details below:

If while developing you introduce a circular dependency between modules, make will let you know when you run cice.build by outputing make: Circular ice_domain.o <- ice_boundary.o dependency dropped. but it does not stop (it is not a fatal error). This message can easily be overlooked since there is a lot of output from cice.build.

Moreover, it is even possible for the build to succeed when ICE_CLEANBUILD is set to false since the modules files already exists. The build is most probably broken in this case because make did not recompile everything that changed sinced it dropped dependencies. If you try a clean build in that case it will always fail.

I added a check in cice.build to directly warn the user if one introduces circular dependencies.

Also, I changed the pipe to the build log from | to |& so that both stdout and stderr are written to the build log when ${quiet}==false

[apcraig]

The circular dependency error message and behavior is likely compiler dependent. The grep implemented in the modifications is probably not general. We probably want to investigate further how to better trap circular dependencies across different compilers and platforms if that is important.

The cleanbuild should be cleaning all .mod, .o, binaries, and the dependencies file. That should be correct and ensure builds are repeatable (and truly clean when rebuilding). If the clean build is not doing that, I think we should try to fix it, not try to trap a circular dependency error created by a bad clean build.

[phil-blain]

Thanks for your feedback.

The circular dependency error message and behavior is likely compiler dependent. The grep implemented in the modifications is probably not general. We probably want to investigate further how to better trap circular dependencies across different compilers and platforms if that is important.

In fact it is an error message outputed by make, and not the compiler. You are right that I opened this PR quickly and did not try to make it as general as I should have. We already have an environment variable ICE_MACHINE_MAKE that is set in the env file. You are right that the error message could be different depending on the make implementation on each machine (or the make version). In our supported machines, ICE_MACHINE_MAKE is set to 'gmake' (which I believe is GNU make) for every machine except travisCI, cesium, fram and millikan (which are my own, JF's and Amélie's workstations) Our ECCC machines run Linux so make is GNU make. I'm pretty sure this is the case for Travis also.

It don't think it's explicitly specified anywhere, bu I'm pretty sure that our Makefile uses GNU Make specific features, so it would not work with BSD make or AIX make or another make implementation (users on these platforms would have to install GNU Make to use our Makefile). So in that case we should just check that the error message outputed by GNU make is the same for different GNU make versions. (And probably we should mention in the user guide that GNU make is required for compiling CICE, maybe in a new 'System requirements' section)

The cleanbuild should be cleaning all .mod, .o, binaries, and the dependencies file. That should be correct and ensure builds are repeatable (and truly clean when rebuilding). If the clean build is not doing that, I think we should try to fix it, not try to trap a circular dependency error created by a bad clean build.

The clean build works fine, that is what I tried to explain above, I might not have been clear enough. The different situations that I try to address are the following:

A (incremental build)

1. Developer creates a new branch for a new feature, creates a case and compiles, setting ICE_CLEANUILD to false to get an incremental build
2. Developer continues working on his branch, periodically running cice.build to recompile his executable.
3. Developer unknowingly introduces a circular dependency in the code between module A and module B (by adding a use statement in module C), and runs cice.build. Make will show the error message and drop dependencies. The compilation will probably succeed because all module files (including A and B) already exist
   => this is what I want to warn the developer about! There should be a clean message at the end of the compilation that says that there are circular dependencies (which I added in this PR). I phrased it as 'WARNING' and not 'ERROR' since the build was able to succeed, but I would like you input on that (I think 'ERROR' would be safer, and even safer would be having cice.build not output 'COMPILE SUCCESSFUL')

B (clean build)

1. Developer creates a new branch for a new feature
2. Developer unknowingly introduces a circular dependency in the code
3. Developer creates a case and compiles from scratch. Make shows the error message and drop dependencies, but the build fails at compiling module B because module A, which is used by module B, does not exist (since Make dropped a circular dependency between A and B and chose to compile B first). The compiler will output an error message saying it cannot find the required modules, which will kill make.
   => I added an 'ERROR' message in this case so that it is more clear that the build failed because of circular dependencies.

[apcraig]

@phil-blain, thanks for the clarifications. Sorry for my confusion.

For case A above, I think if anyone is modifying code and changing the dependencies, they should NOT be doing incremental builds. I don't believe the CICE make system will build the system properly in this case, and the circular dependency problem just seems to prove that it won't support that level of incremental build. The circular dependency problem is just one of many issues that can break when incremental builds are taken too far. I believe the incremental build is best used when small code changes are introduced during the development process. It should not be used for bigger changes, like dependency changes. Maybe we should improve the build process to make this all more robust, but I think there are tradeoffs to that approach and most people will just have cleanbuild on all the time which is the default. Rather than introduce extra checks, traps, and error messages in the build process to work around these issues, I suggest we try to make the incremental build limitations clearer in the documentation and/or add some checks in the build to do something like always run the dependency generator and compare that output to the prior build and have the build stop (or clean automatically) if the dependency changes. Again, I think what we want to do is discourage incremental builds when dependencies change. And if we do that, then the extra check on the circular dependency is not needed. Having said that, I'm also willing to consider improving the robustness of the incremental build feature as the alternative.

For case B, an extra error output can be added, although I'm not sure it's needed in this case as the error should be relatively clear in the build log file.

I think changing the | to |& is a good idea.

[eclare108213]

This problem still catches me from time to time, and I know students who have lost days because of it. Although it's not necessary and shouldn't ever happen, having an extra warning at the end of the compile sequence wouldn't hurt, if the script can be made general enough to work for all compilers. I also agree that stronger language in the documentation regarding incremental builds would be wise.

[apcraig]

There are several tasks that come out of this

• improve documentation about cleanbuild.
• add ability to check if dependency files changed when cleanbuild is false and then either stop the build or clean the build automatically first. that will take care of case A above.

It's not clear to me that the extra error message is really needed in case B. The model will fail as soon as the circular dependence is encountered and it will be the "last error" in the build log file. That seems adequate. It's really case A that creates havoc and is more difficult to detect.

My preference is to close this PR and work on the two bullets above. I'm happy to take those on. Alternatively we could merge this PR, but I would like to be able to remove the extra error messages when the bullets above are addressed. A final alternative is we keep this open and continue to develop. I could PR some changes to @phil-blain's branch to help this evolve. Lets make a plan. @eclare108213 @phil-blain what do you think?

[phil-blain]

I have a few comments :
@apcraig, you write

For case A above, I think if anyone is modifying code and changing the dependencies, they should NOT be doing incremental builds. I don't believe the CICE make system will build the system properly in this case, and the circular dependency problem just seems to prove that it won't support that level of incremental build.

I disagree: the build system works perfectly fine in incremental mode if the user introduces new dependencies (the automatic dependency generation runs each time make is run and it works correctly). I use it all the time this way, and never have any problem. The thing is it just does not prevent the user from introducing circular dependencies. I think it should; the build should detect circular dependencies and simply abort with a clear error message if it finds some. I don't think that automatically cleaning if dependencies change is a good way forward.

It's not clear to me that the extra error message is really needed in case B. The model will fail as soon as the circular dependence is encountered and it will be the "last error" in the build log file. That seems adequate.

Like @eclare108213 said, it can be easy to miss: the last message will be "module x not found", but one has to go to the beginning of the log file to find the "make : circular dependency dropped" message and from that infer that a circular dependency was introduced.

[phil-blain]

So what I propose is :
Case B : leaving the error message as is implemented above.
Case A : instead of simply echoing 'Warning" as I implemented above, I suggest that if circular dependencies are detected, but the build still succeeds, cice.build should delete the created executable and end with an explicit error code (and echo and error saying the circular dependencies have been detected, as in case B).

[phil-blain]

Also, I I think (I haven't check) that all these situations are already implemented in more advanced/modern build systems such as Cmake

[eclare108213]

@phil-blain @apcraig
You two have a good handle on the issues here and seem to be converging. If there's a remaining question that you'd like my input for, please lay it out. My experience is that the circular dependency is not always caught immediately and the module-not-found error message can be unenlightening for users who haven't encountered it before (or recently), so it would be nice to add that (circular dep) message to our output. @phil-blain's proposal directly above seems like a reasonable compromise, if it can be done in a compiler-generic way and there aren't undesirable side-effects of deleting the executable.

[apcraig]

OK, I'm starting to be convinced. Would it be possible to abort as soon as the circular dependency is detected? In other words, rather than having cice.build grep for a string in the build log and do something, could we add a check directly in the Makefile so as soon as the circular dependency is hit, it fails. That would take care of A and B and stop the compile where the error is obvious and never generate a binary. I have googled for a flag in gmake that might do that, but have not found anything yet. Could we add a rule to the Makefile? If so, I think that might be a cleaner way to handle it. @phil-blain, do you think that is possible/reasonable, and if so, do you have time to do some prototyping or would you like me to try to find some time? If that doesn't work, then I'm fine moving ahead with what @phil-blain is proposing.

Separately, I agree that we might depend on gmake and should make that clear in our documentation. Whether we should switch to a different Make system is a reasonable question. gmake is generally the default make option on most systems or it's at least available and lots of folks are familiar with it. I think we have to ask the standard questions if we change to cmake; is it readily available, are people familiar with it, how much effort is it to change, how much will it cost longer term relative to not switching, how much benefit will it provide. I don't know the answer to all those questions, but we could look into it.

[phil-blain]

I also googled for a flag to make make abort if it finds circular deps; I don't think it exists (make was originally developed more for C/C++ and with these languages circular deps are usually not fatal, usually they just come from a badly written makefile) I guess that is why make does not consider it a fatal error.

It might me possible to write a rule, so that the general flow would be
generate dependencies -> check for circular deps -> [build|abort]
but I don't think it would be easy. In fact I can't think of an easy way to do that on the top of my head, that's why I suggested deleting the executable after the grep. I can do some prototyping but not for the next two weeks. I'm going to a conference the week of 18-23 so when I return I can tackle this. If you have time you can go ahead but I don't think this is urgent if you have other stuff. I'd be happy to do it.

Regarding switching to another build system, all these questions are good ones. I know that CMake is becoming the industry standard pretty fast for C/C++, and that they (Kitware and the community) are committed to supporting Fortran also. I'm not familiar with it though, so I don't know how much of an effort it would be to port our build system to it.

[apcraig]

Thanks @phil-blain. If I get a chance, I might try a few things. My thinking was more along the lines of catching the "circular" dependence in the Makefile as part the build, not necessarily building some other logic. Some maybe in the .o: .F there is a rule we can add that can catch the circular dependence, even with a grep-like construct or something. It depends when that error is triggered in the make and whether we can catch it robustly.

[apcraig]

I played around with this branch a bit and now understand much better the issues and problems. I agree this is a valuable feature. Thanks @phil-blain for your patience with me. It is not possible for the Makefile to trap this because it's not an error that the actual build generates, it's an produced by gmake separate from the build. It's an interesting interaction.

I am going to propose some changes via a PR to the branch in a few minutes,

• update logic to trap the circular dependency. There is now one section of code for all traps and it removes the cice binary if it exists.
• update documentation
  • add small section in developers guide about how build works
  • add section in user guide about software requirements
  • add pointer in quick start to software requirements section
  • piggy backed unrelated known bug issue related to history and restart on first timestep, CICE #289

The branch including the PR has been tested on conrad here,
#2cdeeb37333b

The documentation has been tested and can be reviewed here, add-circular-dep-warnings doc build. The main new documentation feature is the software requirements section. We'll have to try to remember to keep it up to date as much as possible, but it will never be complete and I think that's OK. Having something is better than nothing.

[phil-blain]

Thanks @apcraig for the PR. I merged it and added 2 commits (mainly changes to capitalization in the doc).
Also I added a condition in cice.build to only 'tail' the build log (in the case of quiet builds) when no circular dependencies are found, since in this case we are not interested in the end of the build log file.

[apcraig]

Thanks for the latest changes @phil-blain. And thanks again for suggesting this feature and being patient with me. I'll merge now.