Skip to content
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

Clean up handling of Legion config options #670

Open
wants to merge 6 commits into
base: branch-24.03
Choose a base branch
from

Conversation

manopapad
Copy link
Contributor

  • Always build Legion with python support and FFI bindings. This way even if a user starts with a default cmake build, then does pip install, the Legion build that cmake did will automatically be compatible with the python layer. We could just change the default to be ON everywhere, but I don't see a reason why we need to support Legion builds w/o python and bindings, but maybe folks have a different opinion.
  • Move unconditional Legion configuration flags out of install.py, and leave a note to put those in the cmake configuration. As is we run the risk that developers will only update install.py and forget to update the cmake-only defaults.
  • Document major required Legion configuration flags, so that users know how to build Legion such that it's compatible with Legate. Automatically check all the ones that Legion exports (others will have to wait until the Legion cmake build starts exporting them).

@manopapad manopapad added the category:improvement PR introduces an improvement and will be classified as such in release notes label Apr 7, 2023
@manopapad manopapad requested review from jjwilke and trxcllnt April 7, 2023 02:38
install.py Show resolved Hide resolved
cmake/thirdparty/get_legion.cmake Show resolved Hide resolved
install.py Show resolved Hide resolved
Copy link

@jjwilke jjwilke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Installation works and checks correctly catch errors.

Comment on lines +75 to +77
if(NOT Legion_USE_Python)
message(FATAL_ERROR "Legion was not compiled with Legion_USE_Python")
endif()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it impossible to build the legate C++ library without Legion built w/ Python bindings. Is this really what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is my attempt at avoiding the following fell-bad scenario:

  • user builds Legion separately, forgets to add python support
  • user builds legate.core C++ bits using cmake directly
  • users does pip install
  • user realizes nothing is working, because they built Legion w/o Python support (and the build process never complained)

I would also be ok with a situation where the cmake build doesn't enforce Legion_USE_Python, and instead that becomes necessary only when you try to install the python pieces of legate.core. Do you think that's possible/preferable? Or maybe you have a different approach to avoid this scenario?

@marcinz marcinz changed the base branch from branch-23.05 to branch-23.07 May 18, 2023 20:12
@marcinz marcinz changed the base branch from branch-23.07 to branch-23.09 July 18, 2023 15:38
@marcinz marcinz changed the base branch from branch-23.09 to branch-23.11 September 26, 2023 00:23
@marcinz marcinz changed the base branch from branch-23.11 to branch-24.01 November 9, 2023 16:52
@marcinz marcinz changed the base branch from branch-24.01 to branch-24.03 February 22, 2024 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants