Add --extrn (-e) option to devbuild.sh

[chan-hoo]

DESCRIPTION OF CHANGES:

• Add an option "--extrn (-e)" to check out the external component to the build script 'devbuild.sh'.
• With this option, a user can skip the first step to check out the external components separately.
• This option will be more useful as the number of the external.cfg files increases to check out extra external components.

TESTS CONDUCTED:

• Build test on Hera and Orion:

1. current build steps:

./manage_externals/checkout_externals
./devbuild.sh -p=hera

2. with the `--extrn (-e)' option in this PR:

./devbuild.sh -p=hera -e=YES

Both 1) and 2) work to build the app.

• Four tests suggested by @christopherwharrop-noaa below:

1. $ devbuild.sh --extrn=NO; devbuild.sh --extrn=NO => FAIL (as expected)
2. $ devbuild.sh --extrn=NO; devbuild.sh --extrn=YES => FAIL (as expected)
3. $ devbuild.sh --extrn=YES; devbuild.sh --extrn=NO => PASS (as expected)
4. $ devbuild.sh --extrn=YES; devbuild.sh --extrn=YES => PASS (as expected)

where 'PASS' means that its result is the same as the test result with the original script as follows:

./manage_externals/checkout_externals
./devbuild.sh -p=hera
./devbuild.sh -p=hera

[christopherwharrop-noaa]

I'm not sure how useful this will be given that the option probably will only work once, for the first build, of a given clone of the App. I've found the manage_externals utility to be very unfriendly and unusable after the first time it is used. So subsequent uses, or first time uses after checkout_externals has already been done manually, will likely fail horribly. What open issue is this trying to solve?

[JeffBeck-NOAA]

@chan-hoo, I assume that once manage_externals has been run once with the -e option, a user could still rerun devbuild.sh without the -e option, and it would work as before, correct?

[chan-hoo]

@JeffBeck-NOAA, yes, you're correct. both (w/ and w/o -e option) should work. This '--extrn (-e)' flag is optional. @christopherwharrop-noaa, I introduce this option to hear from others before I open a PR to merge RRFS-CMAQ into SRW App. As I mentioned in your PR #269, I used the '-e' flag option of manage_externals to separate the additional external components for RRFS-CMAQ. In this case, we should run the 'manage_exteranls/checkout_externals' command twice or more (if other options like DA are included). With this option, we just need to add "-e=YES". As I mentioned, this is not a mandatary option. If a user prefers checking out the external components manually, the user doesn't need to use this flag. This option is very useful at least for me, so I'd like to introduce it to others :) :) As you said, this is used once for the first build time. Its default value is "NO (false)". You don't need to worry about this option when the script runs if you don't want to use it.

[RatkoVasic-NOAA]

Since this is not required option, it won't affect anyone who is not using it.

[christopherwharrop-noaa]

@chan-hoo - What happens if checkout_externals has already been run and someone uses devbuild.sh with the --extern=YES option? Please edit the description of this PR to include a detailed description of what this is for, why it improves the build process, and what the various expected use cases are. Please also include in the "tests" section the results of running the four following tests:

1. $ devbuild.sh --extrn=NO; devbuild.sh --extrn=NO
2. $ devbuild.sh --extrn=NO; devbuild.sh --extrn=YES
3. $ devbuild.sh --extrn=YES; devbuild.sh --extrn=NO
4. $ devbuild.sh --extrn=YES; devbuild.sh --extrn=YES

@RatkoVasic-NOAA - I'm assuming you mean that existing behavior is unchanged unless someone sets --extrn=YES. I still need a compelling case for why this option is needed with a description of what problem it is solving and why the existing build instructions/methods are not sufficient for handling that problem. I also need assurance that the new option will work correctly for all states of the build and checkout process so that users are not surprised and confused by broken behavior.

[chan-hoo]

@christopherwharrop-noaa, I don't understand what the four cases mean exactly. Can you explain them in detail?

[christopherwharrop-noaa]

@chan-hoo :

1. From a fresh clone run devbuild.sh --extrn=NO and then, when it is done, immediately run devbuild.sh --extrn=NO
2. From a fresh clone run devbuild.sh --extrn=NO and then, when it is done, immediately run devbuild.sh --extrn=YES
3. From a fresh clone run devbuild.sh --extrn=YES and then, when it is done, immediately run devbuild.sh --extrn=NO
4. From a fresh clone run devbuild.sh --extrn=YES and then, when it is done, immediately run devbuild.sh --extrn=YES

Please add any other necessary options such as for platform or compiler to the four tests. The only one of those four tests which I expect to pass is #3. The point is to show that the use of the proposed option assumes a particular state of the cloned repository, and that state is not checked or accounted for in the implementation of the proposed option. I don't see how adding this option, which only works in one very particular situation, and that only occurs once in the lifetime of a given clone of the repository, improves anything. What is the point of this? To replace typing ./manage_externals/checkout_externals with --extrn=YES one time? That is not a convincing case for this feature.

[chan-hoo]

@christopherwharrop-noaa , so in your test cases, does 'pass' mean to see the 'R/C/Q' choice message?

[christopherwharrop-noaa]

@chan-hoo - "pass" means ./devbuild.sh .... runs to completion and produces the expected binaries.

[chan-hoo]

@christopherwharrop-noaa, if you intended it, your test cases were flawed. "From a fresh clone run devbuild.sh --extrn=YES and then, when it is done, immediately run devbuild.sh --extrn=NO", Instead of immediately running the second devbuild.sh, the build and bin directories should be removed before the second run to get the result you want. Anyway, this PR got the same results as posted in the "TEST conducted" section. I think this is good enough for this PR.

[christopherwharrop-noaa]

@chan-hoo - Thank you for taking the time to run those tests. I'm not sure why you think the cases were flawed. Removing the bin/ and build/ directories is not necessary to get the expected behavior (you can choose either to use --clean or continue the build and the result is the same) and I've proven that by doing the same tests on my own on Hera. The tests were designed to illustrate that the --extrn option, as implemented, would not always work correctly. It is not enough to test only the "happy path". You also must test the "unhappy" path to ensure that the expected errors occur and that when errors do occur, they are handled appropriately. It is well known that manage_externals is very sensitive to pre-existing checkouts and does not behave well under those circumstances, and that is what I was wanting to expose. Since then, you've added a check to skip the checkout of external components if regional_workflow exists. This works around the issue pretty nicely, but it, too, relies on a number of assumptions, the most obvious one being that the existence of regional_workflow necessarily implies that all the external components are already checked out. While that might seem like a pretty safe assumption, it is still an assumption, and is not guaranteed, and leaves open the door to erroneous behavior when a user has some components checked out, but not others. It also hugely complicates the maintenance of the implementation of --extrn when other Externals.cfg files are added because you'll then have to keep track of and test for existence of those components as well. There is also the problem of the semantics of the --extrn option and its assumed behavior by those reading its description and using it. It is documented that the option will check out external components. But now, it only checks out externals if they haven't already been checked out. What if the user want to re-checkout the components? The option description seems to indicate that it would do just that, and that would be a reasonable expectation for it. The user will then be very disappointed and confused when they find out, it does absolutely nothing because the components were already checked out. So, what, exactly, does --extrn do? Does it check out externals or doesn't it? As it stands, sometimes it does, and other times, it doesn't. It's ambiguous and misleading. In my reading of this change, the --extrn option, is only useful for one particular scenario which normally happens only once in the entire lifetime of a particular clone of a repository and its weakly defined behavior opens the door for unexpected results. Thank you for adding the bullets describing with a bit more detail what this is for. But, I remain unconvinced about this option's current and future utility given the wide range of issues it brings. Moreover, the documentation for the devbuild.sh script states unambiguously that it is for developer use only and that users should see the official documentation for build instructions.

NOTE: This script is for internal developer use only;
See User's Guide for detailed build instructions

Given the knowledge developers are expected to have in order to work with the SRW App, I really don't see any value here. In my opinion this is just adding unnecessary complexity, maintenance costs, and exposure to unexpected behavior for no discernible benefit.

[chan-hoo]

@christopherwharrop-noaa, that was what I asked in my previous comment: "@christopherwharrop-noaa , so in your test cases, does 'pass' mean to see the 'R/C/Q' choice message?" where R: remove, C: continue, Q: quit.

[chan-hoo]

@christinaholtNOAA, @danielabdi-noaa, @gsketefian, @BenjaminBlake-NOAA, can you review this PR?

[christopherwharrop-noaa]

@chan-hoo - It doesn't matter whether you choose "R" or "C", the result is the same because neither cleaning or continuing impacts the checkout or the behavior of --extrn. I didn't specify the choice there because I thought it was implied that you'd have to choose one, and for this test it doesn't matter which one you pick.

[chan-hoo]

@christopherwharrop-noaa, as I posted in the "TESTS" section above, I got the same result with the original script. I don't understand what more we need. I'd like to hear from other reviewers. If they think this PR is not necessary, I'll close this PR without objection. I don't want to waste my time as well as your valuable time any more.

[BenjaminBlake-NOAA]

I was able to build the app successfully with both options 1 and 2. Since -e is not a required option, I am not opposed to adding this feature if others find it useful.

[christopherwharrop-noaa]

It is worth reminding developers and reviewers that the standard for acceptance of proposed changes is the same whether the new feature is "optional" or "required". In both cases, the new behavior should be both correct and be what users and developers expect for all foreseeable circumstances, including those that are expected to produce errors. The fact that something is "optional" does not exempt it from adherence to standards of quality and utility.

Non-preferred Reviewer

[chan-hoo]

Upon further discussion with the EMC management, we've decided to close this PR.