Change build scripts to accept an optional cmake version argument #1980
Conversation
If no cmake version is given, a default based on the RAJA cmake min version is set
rhornung67
requested review from
MrBurmark,
adayton1,
artv3,
johnbowen42,
rchen20 and
smithsg84
| # Detect optional second positional argument as a CMake version if it looks like N.M or N.M.P | ||
| # Otherwise, treat it as a normal CMake argument. | ||
| if [ -n "$2" ] && [[ "$2" =~ ^[0-9]+(\.[0-9]+)*$ ]]; then | ||
| CMAKE_VER=$2 | ||
| shift 2 | ||
| else | ||
| CMAKE_VER=$DEFAULT_CMAKE_VER | ||
| shift 1 | ||
| fi |
There was a problem hiding this comment.
Would it be more clear if the cmake version was an optional named argument like script compiler_ver [--cmake ver] [cmake args...]? Would putting it first be more uniform script [--cmake ver] compiler_ver [cmake args...]?
There was a problem hiding this comment.
Maybe, but we don't use named args for input to any of the scripts.
I put it last because it's optional and in most cases, it probably won't be needed.
This PR is intended to deal with the specific issue that the CMake version is currently hard-coded in the scripts and we need to be more flexible and it's a pain to modify the scripts for minor changes like that.
There was a problem hiding this comment.
I think in general named arguments are better, and that it would be better if all the build scripts had only named arguments (ideally in Python). However, given that these scripts are used primarily internally, I think it would probably be better to make this as minimal of a change as possible.
In terms of improving usability of these scripts, I think the best thing would probably be a Python driver that calls the shell scripts automatically, and provides users with a nice --help description.
There was a problem hiding this comment.
I agree with that. If you feel inclined to pursue that, please do so.
My intent with this PR was to do a quick, simple change to make the scripts more useful w.r.t. a particular issue.
|
|
||
| # Detect optional second positional argument as a CMake version if it looks like N.M or N.M.P | ||
| # Otherwise, treat it as a normal CMake argument. | ||
| if [ -n "$2" ] && [[ "$2" =~ ^[0-9]+(\.[0-9]+)*$ ]]; then |
There was a problem hiding this comment.
Maybe it would be better to have an additional clause like this
elif [ -n "$2" ] && [[ "$2" !=~ ^[0-9]+(\.[0-9]+)*$ ]]; then
echo "Unable to parse CMake version number"
There was a problem hiding this comment.
Given that this argument won't be used widely I think this help message is not strictly necessary
There was a problem hiding this comment.
I agree that we can make the scripts much more helpful. However, they are specific to LC systems and used mostly for our own development purposes. Sometimes, I point people to them for examples.
|
We'll have to change the RAJAPerf scripts too. |
Yes. I wanted to run this by folks first. |
Summary