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

feat: Add PIP_OPTIONS CMake option for greater control of Python bindings install #1648

Merged

Conversation

matthewfeickert
Copy link
Contributor

@matthewfeickert matthewfeickert commented Mar 14, 2022

This PR will:

  • Add CMake option PIP_OPTIONS to allow the user to pass in pip options at build configuration time to customize the Python bindings install. As --verbose is a pip option, this makes the PIP_VERBOSE CMake option added in PR fix: Install Python bindings with pip if available #1586 redundant, and so it is removed. Additionally, as --prefix is also a pip option, append the selected default value of $ENV{DESTDIR}/{CMAKE_INSTALL_PREFIX} as a --prefix pip option if there is no --prefix given by the user. If there is a given --prefix from the user in PIP_OPTIONS already, warn the user that this might not be a good idea unless they know what they're doing.

  • Remove PYTHON_LAYOUT from override_dh_auto_configure as not used in Python 3 builds.

I would personally be in favor of this option over PR #1647 as it makes things explicit at build time and so much more transparent for debugging, comparisons, and reproducibility.

@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch 3 times, most recently from 4fe7763 to 2935702 Compare March 14, 2022 07:57
@matthewfeickert matthewfeickert changed the title feat: Add PIP_OPTIONS CMake option to supersede PIP_VERBOSE option feat: Add PIP_OPTIONS CMake option for greater control of Python bindings install Mar 14, 2022
@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch from 2935702 to 41715a4 Compare March 14, 2022 08:40
@matthewfeickert matthewfeickert marked this pull request as ready for review March 14, 2022 08:47
@matthewfeickert matthewfeickert marked this pull request as draft March 14, 2022 08:55
@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch from 41715a4 to 71a0491 Compare March 14, 2022 09:36
@matthewfeickert matthewfeickert marked this pull request as ready for review March 14, 2022 09:51
@matthewfeickert matthewfeickert marked this pull request as draft March 14, 2022 10:01
@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch from 71a0491 to 60c434a Compare March 14, 2022 10:04
@matthewfeickert matthewfeickert marked this pull request as ready for review March 14, 2022 10:04
@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Mar 14, 2022

@simonmichal This should be ready for review (assuming the rpm-centos7 CI passes).

@adriansev If you can take a look at this as well that'd be great. In future releases you should definitely be able to have things working in a way that solves Issue #1641 -DPIP_OPTIONS="--ignore-installed" but you might also be able to have things work by just not using the PIP_OPTIONS flag at all, given the removal of --force-reinstall by default in this PR.

@adriansev
Copy link
Contributor

@matthewfeickert so, i checkout out the PR and gave it a spin:

  1. without any additional options the python bindings are not installed and i get:
Processing ./bindings/python
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'
ERROR: Could not find a version that satisfies the requirement OFF (from versions: none)
ERROR: No matching distribution found for OFF

i suspect that some defaults (empty variable) are not well enough processed

  1. when specifying -DPIP_OPTIONS="--ignore-installed" everything just worked, so YUPII :)

@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch 2 times, most recently from 5b73f81 to 2fe7ed1 Compare March 14, 2022 17:39
@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Mar 14, 2022

@adriansev Can you try again without any additional options for the Python bindings? I think the correction @henryiii provided will fix things for you now (e.g. PIP_OPTIONS is now properly a STRING by default and not a BOOL if no input is passed).

Though as he notes, please clean your cache first if you're building locally (e.g. cmake --build <build dir> --clean-first)

@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch from 2fe7ed1 to 36143a1 Compare March 14, 2022 17:49
@adriansev
Copy link
Contributor

@matthewfeickert tried again without any customization on my side, and seems to work like expected, meaning that the wheel was created, my user side xrootd (.local/lib/python3.10/site-packages) was removed (as expected i think without ignored-installed) then the new wheel was installed in the defined private location. From my side i would say that everything works and it's ready to be merged. Thanks a lot for all your work!

@matthewfeickert
Copy link
Contributor Author

From my side i would say that everything works and it's ready to be merged.

Fantastic! Hopefully this is the last time you and ALICE get hit with some strange behavior from XRootD Python bindings. :)

@simonmichal As @adriansev has verified things are working for the problems raised in Issue #1641 this is ready for review. 👍 Let me know if anything isn't clear!

matthewfeickert and others added 2 commits March 15, 2022 21:43
Add CMake option PIP_OPTIONS to allow the user to pass in pip options
at build configuration time to customize the Python bindings install.
As '--verbose' is a pip option, this makes the PIP_VERBOSE CMake option
added in PR 1586 redundant, and so it is removed. Additionally, as '--prefix'
is also a pip option, append the selected default value of
'$ENV{DESTDIR}/{CMAKE_INSTALL_PREFIX}' as a '--prefix' pip option if there is
no '--prefix' given by the user. If there is a given '--prefix' from the user in
PIP_OPTIONS already, warn the user that this might not be a good idea unless
they know what they're doing.

Adrian provided the idea of providing the user a PIP_OPTIONS CMake variable at build
configure time.
Henry provided help on getting PIP_OPTIONS to be recognized as a string by defult.

Co-authored-by: Adrian Sevcenco <adrian.sevcenco@cern.ch>
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
Remove PYTHON_LAYOUT from override_dh_auto_configure as not used in
Python 3 builds.
Use PIP_OPTIONS in the build tests in CI to pass the '--verbose' flag.
As an example of how multiple pip options can be passed as a single string
to PIP_OPTIONS, show in one test that '--force-reinstall' and '--prefix'
options can be used.
@matthewfeickert matthewfeickert force-pushed the feat/add-pip-options-cmake-option branch from 36143a1 to 16343d2 Compare March 16, 2022 02:43
@simonmichal
Copy link
Contributor

@matthewfeickert : again, thanks a lot for all your work :-)

@simonmichal
Copy link
Contributor

P. S. it seems there's a glitch on GitHub, I'll merge it later on today.

@matthewfeickert
Copy link
Contributor Author

P. S. it seems there's a glitch on GitHub, I'll merge it later on today.

Yeah, GitHub basically was down for everyone today. 🤪 One of those days I guess and probably a tough one for GitHub engineering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CMake flag PIP_OPTIONS to allow for passing specific options to pip during the install
4 participants