Do not allow ./create_clone --keepexe if the case is not built

[gold2718]

Currently, ./create_clone --keepexe issues a warning if the case is not built but makes the clone anyway. This can leave an open vulnerability because PIO sometimes changes the valid_values of PIO_TYPENAME during its build. The issue is that the clone retains invalid PIO options which cause runtime errors if used. I see two possible solutions (listed in my order of preference):

1. Fix config_compilers.xml and its use to have the valid_values for PIO_TYPENAME be correct at case.setup time. For instance, if a compiler/machine combo has no <PNETCDF_PATH> tag, then pnetcdf should not be a valild value. A new tag specifying supported NetCDF options (e.g., netcdf4c) could be used to sort through those options. Perhaps, a negative tag (e.g., <NETCDF_UNSUPPORTED_OPTIONS>) could be used to only eliminate unsupported options if that is less common. In this case, we do not need to restrict create_clone since the clone will be valid.
2. Do not allow ./create_clone --keepexe if the case is not built and re-implement system_test_compare_two to work with the correct sequencing.
3. Option 2 but add an argument to the case.create_clone python method to allow an override for testing (to be used by system_test_compare_two). This option does not require a rewrite of system_test_compare_two

[billsacks]

I like the idea of not having pio's build update valid_values, which I think is what you're saying with option (1) - if this change is feasible.

You say that the PIO_TYPENAME valid_values would then be correct at case.setup time. Would they actually be correct right after create_newcase? If not, then there would still be a limitation that you couldn't run create_clone --keep-exe until after running case.setup - but I'd see that as a reasonable restriction, and it would be pretty easy to rework system_test_compare_two to handle that new restriction.

[jedwards4b]

I don't like option 1, it puts/requires information into config_compilers which could be inconsistent
with reality. I prefer option 2 but think that option 3 is also a possibility.

[gold2718]

I think option 1 (my preference) would involve (some version of) the XML changes I listed plus modification to the newcase code to make sure the case env files are correct.

@jedwards4b , what is the point of having supported machines and compilers if the information is 'inconsistent with reality'? PIO will still perform a check by actually finding the requested libraries but this is a way to document what is supported without having to build a case.

[jedwards4b]

At this time we do not need the pnetcdf path in config_machines or config_compilers and we don't need to say in those files whether or not netcdf is built with parallel support - all of these things are determined by pio during the build step. Your suggestion would require us to add fields to one of these two files to explicitly provide this information for every machine. My preference would be not to do that.

[billsacks]

Is this issue actually more general than the title suggests? I'm thinking that really this issue should read: "PIO_TYPENAME not checked properly if you xmlchange it before doing a build". ./create_clone --keep-exe is one specific instance where everything you do is done before that case's build, but it seems that the same issue would apply to a case created with create_newcase. (What happens now if you set PIO_TYPENAME before running case.build to some value not supported on that machine? And is there the possibility that valid_values would be too restrictive before running the build, and then extra valid values are added during the build? The second case seems more problematic than the first, because it would mean that you are required to wait until after the build to change PIO_TYPENAME to certain values.)

In general, it seems we allow things in env_run.xml to be set at any time: you're not expected to wait until after the build to set env_run.xml variables or query their valid values. So this is a departure from the norm. I find it confusing that valid_values in env_run.xml would be updated during the build phase, because (as a user) I'd like to know that I can explore possible settings before doing the build.

On the other hand, I see Jim's point that it's nice to not have to specify this stuff explicitly for each machine, instead relying on the smarts in the pio build script.

So I'd like to suggest my own three options... my preference is for (4)

(4) Don't try to enforce machine-specific restrictions to PIO_TYPENAME via the valid_values mechanism: Make valid_values include everything that could possibly be supported (i.e., do not update valid_values during the pio build), and defer the machine-specific checks to runtime. This feels simple, consistent and intuitive. It seems rare that someone will try to use a PIO_TYPENAME not supported on their machine, and I feel it's okay to defer the check of this to runtime.

(5) Maintain the status quo, which - while more complicated than my preferred option of (4) - doesn't seem that bad.

(6) Do pio's cmake step during case.setup rather than case.build. (This would be consistent with the unix separation into configure vs. build.) Create a new xml file (env_run.xml?) at this time that holds entries determined during the pio cmake (i.e., configure) step. This would be less confusing, because there would be no possibility of thinking that you can change PIO_TYPENAME before running pio's configure - because there would be no PIO_TYPENAME to change at that point. (I suggest doing this during case.setup rather than case.build because I think it's reasonable to ask that the user run case.setup before making all their changes, but I do not think it's reasonable to ask that the user run case.build before making all their changes.)

[gold2718]

I don't think that the PIO build would ever add a new valid value -- the XML file starts off with all possible values as valid.
The problem with 5. is that system_test_compare_two is broken when the default value for PIO_TYPENAME turns out to be unsupported.

What happens now if you set PIO_TYPENAME before running case.build to some value not supported on that machine?

It silently changes it to something that is supported.

This brings me to option 7 which is to simply do that at runtime. @jedwards4b, is there some way to have PIO back off (say to plain netcdf) if the requested option is not available?

[billsacks]

Oh, I didn't realize that the default value can be unsupported. That does make (5) a problem. I don't like having values changed out from under you silently, either at build time or at run time, but I guess I can live with it.

[jedwards4b]

I would vote for only doing this on master after the release branch has been created - not putting it in cesm2 - is that okay?

[gold2718]

Sounds like a plan since it is not obvious how deep this rabbit hole is.

[mvertens]

I definitely agree.

…

On Fri, May 5, 2017 at 10:18 AM, goldy ***@***.***> wrote: Sounds like a plan since it is not obvious how deep this rabbit hole is. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1334 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlxE-veWU2zYjRcnC2YavxNKYDm1Bj6ks5r20vPgaJpZM4M6mmh> .

[github-actions]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.