-
Notifications
You must be signed in to change notification settings - Fork 112
Change build scripts to accept an optional cmake version argument #1980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
531084a
58ad0cc
dd8dd93
f0c5b8a
0e72c76
3d8ca66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,25 +9,38 @@ | |
| # SPDX-License-Identifier: (BSD-3-Clause) | ||
| ############################################################################### | ||
|
|
||
| if [[ $# -lt 1 ]]; then | ||
| echo | ||
| echo "You must pass 1 argument to the script: " | ||
| echo " 1) SYCL compiler installation path" | ||
| # Default CMake version if not provided | ||
| DEFAULT_CMAKE_VER=3.25.2 | ||
|
|
||
| # Require compiler version | ||
| if [ "$1" == "" ]; then | ||
| echo | ||
| echo "For example: " | ||
| echo " corona_sycl.sh /usr/workspace/raja-dev/clang_sycl_16b7bcb09915_hip_gcc10.3.1_rocm6.4.3" | ||
| exit | ||
| echo "You must pass a SYCL compiler path to script. For example," | ||
| echo " corona_sycl.sh /usr/workspace/raja-dev/clang_sycl_16b7bcb09915_hip_gcc10.3.1_rocm6.4.3 [3.27.4]" | ||
| echo "An optional second argument can be given to set the CMake version to load." | ||
| echo "If no CMake version is provided, version ${DEFAULT_CMAKE_VER} will be used." | ||
| exit 1 | ||
| fi | ||
|
|
||
| SYCL_PATH=$1 | ||
| shift 1 | ||
|
|
||
| # 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 | ||
|
Comment on lines
+27
to
+35
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more clear if the cmake version was an optional named argument like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| BUILD_SUFFIX=corona-sycl | ||
| : ${BUILD_TYPE:=RelWithDebInfo} | ||
| RAJA_HOSTCONFIG=../host-configs/lc-builds/toss4/corona_sycl.cmake | ||
|
|
||
| echo | ||
| echo "Creating build directory build_${BUILD_SUFFIX} and generating configuration in it" | ||
| echo "Using CMake version: ${CMAKE_VER}" | ||
| echo "Configuration extra arguments:" | ||
| echo " $@" | ||
| echo | ||
|
|
@@ -40,7 +53,7 @@ DATE=$(printf '%(%Y-%m-%d)T\n' -1) | |
| export PATH=${SYCL_PATH}/bin:$PATH | ||
| export LD_LIBRARY_PATH=${SYCL_PATH}/lib:${SYCL_PATH}/lib64:$LD_LIBRARY_PATH | ||
|
|
||
| module load cmake/3.23.1 | ||
| module load cmake/${CMAKE_VER} | ||
|
|
||
| cmake \ | ||
| -DCMAKE_BUILD_TYPE=${BUILD_TYPE} \ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to have an additional clause like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.