[REVIEW] ENH Allow arbitrary CMake config options in build.sh

[dillon-cullinan]

This PR adds a feature to the root build.sh script which allows it to consume any arbitrary CMake args and add them to the CMake configuration step.

Some local testing:
--cmake-args

$ bash build.sh -v -g --cmake-args='improper format' libcudf
Invalid formatting for --cmake-args, see --help: --cmake-args=improper format

$ bash build.sh -v -g --cmake-args="improper format" libcudf
Invalid formatting for --cmake-args, see --help: --cmake-args=improper format

$ bash build.sh -v -g --cmake-args= libcudf
Invalid formatting for --cmake-args, see --help: --cmake-args=

# This is just echoing the additional args that will be added to cmake then exiting before build
$ bash build.sh -v -g --cmake-args=\"-DEXAMPLE_ARG -DEXAMPLE_ARG=/path/to/file -DEXAMPLE_ARG=\"string argument\"\" libcudf
-DEXAMPLE_ARG -DEXAMPLE_ARG=/path/to/file -DEXAMPLE_ARG="string argument"

New --help output

$ bash build.sh --help
build.sh [clean] [libcudf] [cudf] [dask_cudf] [benchmarks] [tests] [libcudf_kafka] [cudf_kafka] [custreamz] [-v] [-g] [-n] [-h] [-l] [--cmake-args="<args>"]
   clean			 - remove all existing build artifacts and configuration (start
                                   over)
   libcudf			 - build the cudf C++ code only
   cudf				 - build the cudf Python package
   dask_cudf			 - build the dask_cudf Python package
   benchmarks			 - build benchmarks
   tests			 - build tests
   libcudf_kafka		 - build the libcudf_kafka C++ code only
   cudf_kafka			 - build the cudf_kafka Python package
   custreamz			 - build the custreamz Python package
   -v				 - verbose build mode
   -g				 - build for debug
   -n				 - no install step
   -l				 - build legacy tests
   --allgpuarch			 - build for all supported GPU architectures
   --disable_nvtx		 - disable inserting NVTX profiling ranges
   --show_depr_warn		 - show cmake deprecation warnings
   --ptds			 - enable per-thread default stream
   --cmake-args=\"<args>\"	 - pass arbitrary list of CMake configuration options (escape all quotes in argument)
   -h | --h[elp]		 - print this text

   default action (no args) is to build and install 'libcudf' then 'cudf'

[ajschmidt8]

I would solicit feedback from @robertmaynard or @trxcllnt. My understanding of build.sh is that it is just supposed to provide sane defaults for users that don't have CMake knowledge to build the projects. I think more advanced users would just call CMake directly, which would defeat the purpose of this PR. Otherwise build.sh just becomes a light wrapper for CMake that we have to maintain.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@d45f108). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3954c2e differs from pull request most recent head 73d7811. Consider uploading reports for the commit 73d7811 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.10    #8996   +/-   ##
===============================================
  Coverage                ?   10.65%           
===============================================
  Files                   ?      114           
  Lines                   ?    19077           
  Branches                ?        0           
===============================================
  Hits                    ?     2033           
  Misses                  ?    17044           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d45f108...73d7811. Read the comment docs.

[dillon-cullinan]

I would solicit feedback from @robertmaynard or @trxcllnt. My understanding of build.sh is that it is just supposed to provide sane defaults for users that don't have CMake knowledge to build the projects. I think more advanced users would just call CMake directly, which would defeat the purpose of this PR. Otherwise build.sh just becomes a light wrapper for CMake that we have to maintain.

The purpose of build.sh is two-fold:

1. A single source of truth for building the code in the repository
2. Simplified usage as you mentioned

This isn't just a script for users, but for automation as well. Any other projects that may utilize cuDF (or other RAPIDS libraries source code) has access to this script which minimizes maintenance across the board. If a docker image is using CMake and other build tools directly, any changes to this process will require changes to multiple areas, which is difficult to maintain, especially as RAPIDS starts expanding. A project that utilizes build.sh will always be up-to-date with any changes to the build process over time.

For example, this PR: 61091a0

This change was one that affected the build commands being invoked. If it weren't for this script existing, we would have to edit these commands everywhere, not only in our own CI scripts (which this is utilized in), but also across our docker repo and any Jenkins builds that may be using this. It would be difficult to track, and the cost of maintenance for this script is much smaller than however many instances of build invocation we have across our infra.

[dillon-cullinan]

rerun tests

[robertmaynard]

In general this option doesn't seem to be work as documented:

All of the following fail:

./build.sh libcudf --cmake-args=-DVALUE=TRUE
./build.sh libcudf --cmake-args="-DVALUE=TRUE"
./build.sh libcudf --cmake-args=\\\"-DVALUE=TRUE\\\"

./build.sh libcudf --cmake-args=\\\"--log-level=VERBOSE\\\"
./build.sh libcudf --cmake-args="--log-level=VERBOSE"
./build.sh libcudf --cmake-args=--log-level=VERBOSE

[dillon-cullinan]

./build.sh libcudf --cmake-args="-DVALUE=TRUE"
./build.sh libcudf --cmake-args=\"-DVALUE=TRUE\"

./build.sh libcudf --cmake-args=\"--log-level=VERBOSE\"
./build.sh libcudf --cmake-args="--log-level=VERBOSE"
./build.sh libcudf --cmake-args=--log-level=VERBOSE

The error was caused by some weird interaction when --cmake-args was the last argument, I was getting error output whenever I had an argument following it. So good catch on this.

The correct formatting is a single escape character, so all of these are expected failures. If the documentation is unclear let me know.

Here's a screenshot of these commands failing in a better way after the latest changes, my environment isn't setup to build cudf so there are CMake errors. I formatted your example in the expected format to show that they do indeed make it to the cmake configuration step.

[dillon-cullinan]

rerun tests

[robertmaynard]

The error was caused by some weird interaction when --cmake-args was the last argument, I was getting error output whenever I had an argument following it. So good catch on this.

New logic is working great locally.

Currently the script silently mangles multiple --cmake-args entries ( ./build.sh libcudf --cmake-args="-DARG" --cmake-args="-DARG2") by capturing all flags/options between the two --cmake-args entries. We should either error out or support this.

[dillon-cullinan]

While testing the latest commit I found that leaving out = in the arg also causes silent errors. I'll fix this as well.

[dillon-cullinan]

@gpucibot merge

[dillon-cullinan]

rerun tests