Skip to content
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.

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

Update circular dep warnings branch #7

Conversation

apcraig
Copy link

@apcraig apcraig commented Aug 10, 2019

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

  • 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 with current updates has been tested on conrad here,
#2cdeeb37333b

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

@apcraig
Copy link
Author

apcraig commented Aug 10, 2019

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

@phil-blain phil-blain self-assigned this Aug 12, 2019
@phil-blain phil-blain self-requested a review August 12, 2019 14:48
@phil-blain phil-blain merged commit 7a49abf into phil-blain:add-circular-dep-warnings Aug 12, 2019
phil-blain pushed a commit that referenced this pull request Nov 16, 2021
@apcraig apcraig deleted the add-circular-dep-warnings branch August 17, 2022 20:57
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