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

Explicitly build Legion if legion_dir or legion_src_dir is not provided #411

Merged

Conversation

trxcllnt
Copy link

@trxcllnt trxcllnt commented Oct 4, 2022

This PR fixes an issue where building and installing legate.core succeeds, but re-building and installing legate.core again (without uninstalling it first) seemingly succeeds but removes the Legion libs.

Here's the output from two runs:

  1. The first run builds and installs Legion as part of building the legate.core wheel.
  2. The second run doesn't build Legion (because the installed version from before was found by CMake)

As part of installing the wheel produced by step 2, pip first removes the files added by the previous installation (step 1), but it doesn't re-install Legion (because Legion was found in an install location and not built as a side-effect of building legate.core.)

This PR forces install.py to tell CMake to always build Legion as part of building legate.core unless the user explicitly passes --with-legion or --legion-src-dir.

@trxcllnt trxcllnt requested review from jjwilke and bryevdv October 4, 2022 18:16
@trxcllnt trxcllnt added category:improvement PR introduces an improvement and will be classified as such in release notes category:bug-fix PR is a bug fix and will be classified as such in release notes and removed category:improvement PR introduces an improvement and will be classified as such in release notes labels Oct 4, 2022
@bryevdv
Copy link
Contributor

bryevdv commented Oct 11, 2022

This seems good to me

@bryevdv
Copy link
Contributor

bryevdv commented Oct 12, 2022

@trxcllnt I pushed the button to merge in the mypy fixes, should go green now :D

@bryevdv bryevdv merged commit c304e1f into nv-legate:branch-22.12 Oct 12, 2022
@bryevdv
Copy link
Contributor

bryevdv commented Oct 12, 2022

merge ok'd by @trxcllnt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bug-fix PR is a bug fix and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants