-
Notifications
You must be signed in to change notification settings - Fork 62
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
Build helper functions for legate projects, legate-hello example #571
Build helper functions for legate projects, legate-hello example #571
Conversation
7d50b4a
to
ba8216e
Compare
Hello world example being developed here: |
d167004
to
a08d963
Compare
@robertmaynard I'd also appreciate your thoughts on this PR. |
@jjwilke can we also add the |
cmake/legate_helper_functions.cmake
Outdated
BUILD_EXPORT_SET ${OPT_EXPORT} | ||
INSTALL_EXPORT_SET ${OPT_EXPORT}) | ||
if (NOT ${target}_FOUND) | ||
add_subdirectory(${dir} ${CMAKE_SOURCE_DIR}/build) |
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 would use something like ${CMAKE_SOURCE_DIR}/legate_${${target}}_build
instead of ${CMAKE_SOURCE_DIR}/build
.
The build
directory could already be used by a target named that, or a user might want to call legate_add_cpp_subdirectory
multiple time and this wouldn't work
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.
@robertmaynard that's actually the point of this line, since this is only executed when running via scikit-build
- If the user configures C++ independently, they can do it into
./build
- If the user lets scikit-build configure the C++ lib as a side-effect of building the Python lib, the C++ libs still end up in
./build
(where normally they'd be under something like_skbuild/<arch>/cmake-build
)
This stability helps with the runtime library loading we do when dev'ing locally w/ pip install --editable .
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 split the difference since it's a valid point that this only supports a single source directory. I put everything in:
${CMAKE_SOURCE_DIR}/build/legate_${target}
- which I think is okay? @robertmaynard
This still makes the libraries easy to find.
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.
@jjwilke that assumes users configuring the C++ library themselves know to build in that dir? my guess is they won't know to do this:
cmake -S . -B build/legate_hello_world && cmake --build build/legate_hello_world
CMAKE_ARGS="-Dhello_world_ROOT=$PWD/build/legate_hello_world" \
pip install --editable .
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 we should make it configurable, or do something like build/${cmake_build_type}
? So that way at least it's build/Debug
and build/Release
?
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.
When using add_subdirectory(${dir} ${CMAKE_SOURCE_DIR}/build)
you are stating that CMake should parse the CMakeLists.txt
in ${dir}
and place any build information in ${CMAKE_SOURCE_DIR}/build
. This has the following issues:
- You are placing build artifacts in the soure tree. The path should start with
CMAKE_BUILD_DIR
.
Sincelegate_add_cpp_subdirectory
is executed by the users project we have no idea what source tree layout they have and shouldn't try to write toCMAKE_SOURCE_DIR
. - The second argument must be unique across a CMake project. So
${CMAKE_BUILD_DIR}/legate_${target}
would be sufficient. So the following logic doesn't work currently:
legate_add_cpp_subdirectory(legate_plugin1 hello EXPORT hello-export)
legate_add_cpp_subdirectory(legate_plugin2 world EXPORT world-export)
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.
Thanks all for the suggestions. Silly mistakes on my part. I believe the latest version correctly puts things into CMAKE_BINARY_DIR
into unique folders.
PTAL. @robertmaynard @trxcllnt
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.
Updated version looks good to me
3afe257
to
4619de9
Compare
@trxcllnt @manopapad Sorry everybody... PTAL. With the recent breakage due to PR #581 I decided the hello-world examples should really be maintained in the legate-core repo with build testing in CI to make sure things don't get broken. The current setup abuses |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
9a2bd85
to
31eef52
Compare
for more information, see https://pre-commit.ci
…ore into hello-world-boilerplate
for more information, see https://pre-commit.ci
…ore into hello-world-boilerplate
…hello-world-boilerplate
* simplify README and move content to docs * split users guide and api ref chapters * missing comma * add contact email
With this set of helper functions, the CMakeLists.txt for a downstream project like cunumeric would now look like:
src/CMakeLists.txt:
CMakeLists.txt: