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

Add PYTHON_INSTALL_PREFIX CMake option #2797

Merged
merged 5 commits into from Jan 24, 2021

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Jan 4, 2021

This comes out of the discussion in #2775.

Right now, the Python bindings will simply be installed to CMAKE_INSTALL_PREFIX, even if PYTHON_EXECUTABLE is set to some other location. This PR does not automatically extract the path of the Python installation from PYTHON_EXECUTABLE, but simply adds a PYTHON_INSTALL_PREFIX option so that a separate location for the Python bindings to be installed can be specified.

@BJWiley233 do you think that you could try these changes and see if they work correctly?

@BJWiley233
Copy link

Yes I will try it today and let you know. Thanks.

@BJWiley233
Copy link

The install with PYTHON_INSTALL_PREFIX=/path/to/anaconda3 worked for me on Ubuntu 20.04. FYI there is still a missing closing ")" on line 233 of https://github.com/mlpack/mlpack/blob/master/src/mlpack/bindings/R/CMakeLists.txt for the closing ")" of the execute_process command. This causes R bindings install to fail unless you manually correct.

@zoq zoq added this to Need Testing in PR Tracking Jan 4, 2021
@rcurtin
Copy link
Member Author

rcurtin commented Jan 5, 2021

@BJWiley233 nice catch, thanks!

@rcurtin
Copy link
Member Author

rcurtin commented Jan 5, 2021

@Yashwants19 just FYI, I fixed the missing brace for the R binding installation in this PR. Let me know if a deeper change was needed. :)

Co-authored-by: Yashwant Singh Parihar <Yashwantsingh.sngh@gmail.com>
Copy link
Member

@Yashwants19 Yashwants19 left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this. This is looking awesome. 👍

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks great to me as well.

@zoq zoq moved this from Need Testing to Done in PR Tracking Jan 14, 2021
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Useful addition!

src/mlpack/bindings/python/CMakeLists.txt Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants