-
Notifications
You must be signed in to change notification settings - Fork 514
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
[REVIEW] Use cmake --build in build.sh to facilitate switching build tools #3487
[REVIEW] Use cmake --build in build.sh to facilitate switching build tools #3487
Conversation
Use cmake --build rather than directly invoking make to better support optional use of other build tools
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.
Changes look good, just had a question/request
cd ${LIBCUML_BUILD_DIR} | ||
make -j${PARALLEL_LEVEL} ${MAKE_TARGETS} VERBOSE=${VERBOSE} ${INSTALL_TARGET} | ||
build_args="--target ${MAKE_TARGETS} ${INSTALL_TARGET}" |
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 can't remember what is the source of ${INSTALL_TARGET}
but isn't it just the word install
? What do you think of removing it and just using build_args="--target ${MAKE_TARGETS} install"
which is a bit easier to follow
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.
At line 127, we can potentially suppress installation with the -n
flag by setting INSTALL_TARGET=""
, so I think we still need it, unfortunately.
@gpucibot merge |
Use
cmake --build
in build.sh rather than invokingmake
directly. This should not affect existing usage at all but will allow developers to build with e.g. Ninja simply by settingCUML_EXTRA_CMAKE_ARGS='-GNinja'
when building.