Conversation
|
@Rick-Methot-NOAA please look at the new warning_ss_ref.txt file. There are some warnings produced by the admb docker image that we haven't seen prior but look like they might be easy fixes. |
|
I saw one warning regarding an unguarded "For". I will fix that. Note that For statements that rely on indentation for interpretation are easily broken. I try to always use explicit {}. |
iantaylor-NOAA
left a comment
There was a problem hiding this comment.
I don't have enough experience with yml or Docker to go through these changes line by line, but as long as the new approach is producing the test results that we expect, that's good enough for me.
You may want to just delete the big commented-out sections of code associated with the old way of doing things. I'm told that this is the best practice where you can rely on the git history to see the old way. However, if having those comments present in the code helps understand what the new code is doing, then feel free to leave them in.
|
@iantaylor-NOAA my thought process for retaining them is to have them easily accessible in case 1) the docker image stops working and 2) we need to test compiling locally across all/multiple OS' (in the case that admb no longer will compile SS3 locally and we need to see if it's a compiler issue or if we need to see if it will compile locally when we finally get updated to windows 11). |
|
Surely someone is already compiling with Win11. Is there significant risk? |
|
I'm not sure at this point. I can delete the commented-out portion that downloads admb and puts it in the path for most of the workflows but leave it in one (probably the build-ss3) just to have easily accessible as a just in case measure, at least until we have a better understanding of what complications could arise? |
|
I think it makes sense to archive it in some readily retrievable form. The source code repo has lots of commits, so if we rely on git history to retrieve this, then we at least should have a label on that commit. |
|
Given this background, I think it's fine to leave the commented-out code in the files for now. If we find that we haven't needed down the road they can always be deleted later. |
|
Okay sounds good. I'll go ahead and merge then! |
Concisely describe what has been changed/addressed in the pull request.
Update all workflows to use the admb docker image.
#563
What tests have been done?
Where are the relevant files?
What tests/review still need to be done?
See PR #552
Is there an input change for users to Stock Synthesis?
Additional information (optional).