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

Revise CMAKE helper functions to support custom Python paths. #702

Merged
merged 2 commits into from
May 16, 2023

Conversation

csadorf
Copy link

@csadorf csadorf commented Apr 25, 2023

This change set enables the support of legate plugin repository structures, where the CPP and Python source code are maintained in different top-level directories and also supports nested Python directories for namespace packages, such as legate/my_lib.

@manopapad manopapad requested review from jjwilke and trxcllnt May 3, 2023 05:40
@manopapad manopapad added the category:improvement PR introduces an improvement and will be classified as such in release notes label May 3, 2023
@csadorf
Copy link
Author

csadorf commented May 3, 2023

@manopapad I think something went wrong with the label job.

@magnatelee
Copy link
Contributor

@manopapad I think something went wrong with the label job.

@csadorf we require that every PR have one of the "category" labels, to generate the release notes automatically later.

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. Check the the SOURCE_DIR vs CURRENT_SOURCE_DIR? Very minor point. If you are a building in a subfolder, do you want the python file generated in the subfolder?

elseif(IS_ABSOLUTE LEGATE_OPT_PY_PATH)
set(py_path "${LEGATE_OPT_PY_PATH}")
else()
set(py_path "${CMAKE_SOURCE_DIR}/${LEGATE_OPT_PY_PATH}")
Copy link

Choose a reason for hiding this comment

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

Should this be CMAKE_SOURCE_DIR or CMAKE_CURRENT_SOURCE_DIR?

Copy link
Author

Choose a reason for hiding this comment

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

Using CMAKE_CURRENT_SOURCE_DIR is probably more flexible.

@csadorf
Copy link
Author

csadorf commented May 9, 2023

@jjwilke I addressed your question.

@csadorf
Copy link
Author

csadorf commented May 15, 2023

@jjwilke Any chance we could merge this soon?

@jjwilke jjwilke merged commit f3d6f76 into nv-legate:branch-23.05 May 16, 2023
@jjwilke
Copy link

jjwilke commented May 16, 2023

@csadorf Done. Sorry about the delay.

@csadorf
Copy link
Author

csadorf commented May 16, 2023

@csadorf Done. Sorry about the delay.

No worries at all, thank you for merging!

manopapad pushed a commit that referenced this pull request Mar 5, 2025
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.

4 participants