Make setting CHPL_* environment variables to invalid values an error

[jabraham17]

Makes most CHPL_* environment variables produce an error if set to an invalid value. This can help prevent user error when accidentally doing the wrong thing.

For example, a user might misspell something or just misunderstand how to set a variable. Previously, we would just silently take that value and maybe break in expected ways later. Now, we refuse to accept it and tell the user exactly what they did wrong

Previously,

export CHPL_LLVM=ssystem
make # error, cannot find Makefile.include-ssystem

Now,

export CHPL_LLVM=ssystem
make # Error: CHPL_LLVM=ssystem is not valid, CHPL_LLVM must be none, bundled, or system

[Reviewed by @mppf]

[bradcray]

Eventually (whether in this PR or elsewhere), I think it'd be interesting to make some of these settings' values based not on a list of values embedded in the script but other artifacts within the source tree. For example, CHPL_COMM's legal values might based on the presence of a subdirectory within runtime/src/comm/* or CHPL_*_COMPILER's legal values might be based on the presence of a make/compiler/Makefile.* file. The goal of such an approach would be to permit someone to create their own clone of a comm layer or compiler Makefile, give it a new name, and test it immediately without editing the error-checking code.

The main downsides that I can think of are (a) would it work outside of the full source tree? (e.g., maybe we only release runtime/include/comm/* and not the sources, so maybe should look there?) and (b) would it increase the time required to do the checks?

[jabraham17]

I think that is a very attractive idea. I am very much in support of only having to specify things once, so inferring the valid values of CHPL_COMM from the runtime source tree is attractive. However, as you point out, that does not work once you get into other release formats that don't have the source tree (linux packages immediately come to mind)

Note that this PR only adds checks for things like CHPL_COMM and CHPL_LLVM that have discrete things values. It does not add checking for things like CHPL_*_PLATFORM or CHPL_*_COMPILER, because its much harder to pin down what a "correct" value for those is and they are easier to change (relative to adding a whole new COMM layer, for example)

[bradcray]

However, as you point out, that does not work once you get into other release formats that don't have the source tree (linux packages immediately come to mind)

We must still ship the runtime headers with our binary release formats, though?

[jabraham17]

Yup, you are correct. I just checked, we do ship the runtime includes

[mppf]

The goal of such an approach would be to permit someone to create their own clone of a comm layer or compiler Makefile, give it a new name, and test it immediately without editing the error-checking code.

IMO we should stick with simple checking for invalid values here. IMO an invalid value error from printchplenv etc won't be hard to fix because it amounts to searching to find the code emitting the error & then adjusting the list. And how often do we add a new comm layer etc? I think we should opt for simplicity & more understandable error checking rather than making something more complex.

[mppf]

@jabraham17 - I can review this PR in its current state but please rebase it to clean up the history. The CI checks are getting tripped up by the big merge commit & a merge shouldn't be necessary for this PR changing ~ 100 lines.

[mppf]

Huh, this checks it even if it was the default. You could do that everywhere but if not IMO it's better to put this up top with the conditional

if gmp_val:
  check_valid_var('CHPL_GMP', gmp_val, ['none', 'bundled', 'system'])
else;
  ... set default ...

Let's pick one strategy and try to use it throughout.

[jabraham17]

I prefer checking only if we don't try to infer something. I will adjust accordingly

[jabraham17]

I take that comment back. When writing this I used always checking more consistently and for good reason. Many of these scripts already have complex logic, this would make it worse. I am going make them all always check the valid values (except for something like checking CHPL_GASNET_SEGMENT only with gasnet)