phil-blain / CICE

Development repository for the CICE sea-ice model
Other
0 stars 0 forks source link

Update circular dep warnings branch #7

Closed apcraig closed 5 years ago

apcraig commented 5 years ago

This proposes some updates to the add-circular-dep-warnings branch including

The branch with current updates has been tested on conrad here, #2cdeeb37333b

The documentation has been tested here, add-circular-dep-warnings doc build

apcraig commented 5 years ago

@phil-blain, just to clarify the new implementation. It does not differentiate between A and B in the CICE PR as we've discussed before. The script checks for a circular dependence error immediately after building. If it's found, it writes error messages, removes the binary if it exists, then sets the bldstat flag to 55 (arbitrary number) which will flag the compile as failed in the script and the build will then fail in the standard way.

It's possible some might find this approach a bit "heavy handed", but I think it gets to the heart of the issues you raise. In most cases, the build would have failed anyway and we just get some extra output. In cases where an incremental build with a new circular dependence does build, it cannot be relied on and should be discarded.

Thanks again for raising this issue. You are right that this is a really good idea. Sorry for being slow and dense to pick up on the subtle aspects. If you have any suggestions for further improving the script, let me know. I'm happy to update my PR to your branch before merging on your branch if you have any further ideas. If you feel keeping the A and B sections separate makes sense, let me know. But it seemed like we were converging to a point where we wanted both A and B to effectively behave the same; trap the error, delete any binary, abort the compile cleanly. Thanks again.