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

Staged installation of Python bindings not working after migration to pip #1768

Closed
amadio opened this issue Aug 23, 2022 · 6 comments
Closed
Assignees
Milestone

Comments

@amadio
Copy link
Member

amadio commented Aug 23, 2022

Original bug report from Gentoo Linux: https://bugs.gentoo.org/861452

Bisection with git points to commit 31e3d71 as the problematic change.

How to reproduce the problem:

$ # Configure with Python enabled and install prefix in the system (e.g. /usr)
$ cmake -S $PWD -B build -DENABLE_PYTHON=1 -DPYTHON_EXECUTABLE=/usr/bin/python3 -DCMAKE_INSTALL_PREFIX=/usr
$ cmake --build build
$ # Perform a staged installation into a DESTDIR which is not empty (e.g. to a directory install in the current directory):
$ env DESTDIR=$PWD/install cmake --build build --target install

What I get:

... (many removed lines of output)
-- Processing: /home/amadio/src/xrootd/install//usr/share/man/man1/xrdcp.1
-- Processing: /home/amadio/src/xrootd/install//usr/share/man/man1/xrdmapc.1
-- Installing: /home/amadio/src/xrootd/install/usr/lib64/libXrdClHttp-5.so
-- Set runtime path of "/home/amadio/src/xrootd/install/usr/lib64/libXrdClHttp-5.so" to ""
Processing ./bindings/python
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'
Building wheels for collected packages: xrootd
  Building wheel for xrootd (setup.py): started
  Building wheel for xrootd (setup.py): finished with status 'done'
  Created wheel for xrootd: filename=xrootd-2022.822+dc680e5f8-cp310-cp310-linux_x86_64.whl size=711034 sha256=ca7888c135d15b360c8f8df1f7c05222855e3bf39bd7e0105ced715581f59d67
  Stored in directory: /tmp/pip-ephem-wheel-cache-k0enpw1z/wheels/4f/bf/c3/60b496a0b3d3cc1bbe87053fb501e707645f2f8833a13d0c18
Successfully built xrootd
Installing collected packages: xrootd
  Attempting uninstall: xrootd
    Found existing installation: xrootd 5.4.3
    Uninstalling xrootd-5.4.3:
ERROR: Could not install packages due to an OSError: [Errno 13] Permission denied: '/usr/lib/python3.10/site-packages/XRootD/__init__.py'
Consider using the `--user` option or check the permissions.

Edit (09/11/2022): There are some existing issues with pip for this behavior: pypa/pip#3063, pypa/pip#1716, and pypa/pip#2677.

@matthewfeickert
Copy link
Contributor

matthewfeickert commented Aug 23, 2022

@amadio You should just use the PIP_OPTIONS CMake option that was added in PR #1648 in your cmake build if you truly need --ignore-installed.

Example:

cmake3 \
-DCMAKE_INSTALL_PREFIX=/usr/local/ \
-DPYTHON_EXECUTABLE=$(command -v python3) \
-DENABLE_TESTS=ON \
-DPIP_OPTIONS="--verbose --force-reinstall --prefix /usr/local/" \
-S xrootd \
-B build

Though I'm skeptical of why --ignore-installed is needed still (c.f. #1769 (comment)). Regardless, can you verify that this option works for the problem you reported?

@abh3
Copy link
Member

abh3 commented Sep 28, 2022

Shall we close this then?

@amadio
Copy link
Member Author

amadio commented Sep 28, 2022

Yes, I think we can close this.

@amadio amadio closed this as completed Sep 28, 2022
@amadio
Copy link
Member Author

amadio commented Nov 4, 2022

I've decided to take a look at this again due to #1807 and because I think that make DESTDIR=$INSTALL install should work by default without having to change $PIP_OPTIONS. The patch below seems sufficient (just add the same --prefix option from the command used on Debian):

--- a/bindings/python/CMakeLists.txt
+++ b/bindings/python/CMakeLists.txt
@@ -95,7 +95,7 @@ else()
     CODE
     "EXECUTE_PROCESS(
       COMMAND /usr/bin/env ${XROOTD_PYBUILD_ENV} ${PYTHON_EXECUTABLE} -m pip install \
-                ${PIP_OPTIONS} \
+                --prefix \$ENV{DESTDIR}/${CMAKE_INSTALL_PREFIX} ${PIP_OPTIONS} \
                 ${CMAKE_CURRENT_BINARY_DIR}
       )"
   )

After applying this, with the default CMAKE_INSTALL_PREFIX=/usr/local I now see things get installed in the right place with a staged installation:

build $ tree -d install
install
└── usr
    └── local
        ├── bin
        ├── include
        │   └── xrootd
        │       ├── XProtocol
        │       ├── Xrd
        │       ├── XrdCl
        │       ├── XrdHttp
        │       ├── XrdNet
        │       ├── XrdOuc
        │       ├── XrdPosix
        │       ├── XrdSec
        │       ├── XrdSys
        │       ├── XrdXml
        │       ├── XrdXrootd
        │       └── private
        │           ├── Xrd
        │           ├── XrdCl
        │           ├── XrdNet
        │           ├── XrdOuc
        │           ├── XrdPosix
        │           ├── XrdSecsss
        │           ├── XrdSys
        │           └── XrdZip
        ├── lib
        │   └── python3.10
        │       └── site-packages
        │           ├── XRootD
        │           │   ├── __pycache__
        │           │   └── client
        │           │       └── __pycache__
        │           ├── pyxrootd
        │           │   └── __pycache__
        │           └── xrootd-2022.1020+3a0998333.dist-info
        ├── lib64
        └── share
            ├── man
            │   ├── man1
            │   └── man8
            └── xrootd
                └── cmake

42 directories

@matthewfeickert What do you think about this?

@amadio amadio reopened this Nov 4, 2022
@amadio amadio assigned amadio and unassigned simonmichal Nov 8, 2022
amadio added a commit to amadio/xrootd that referenced this issue Nov 8, 2022
The problem is that with a single slash as in \$ENV{DESTDIR}, the variable
gets expanded too early (i.e., when passing ${PIP_OPTIONS} in the call to
install(CODE...)), so when "make DESTDIR=$INSTALL_DIR install" is run, the
variable is not part of the recipe, as $ENV{DESTDIR} has been passed to
install(CODE..) and has been expanded into an empty value. By adding one
more level of quoting, the expansion of DESTDIR happens at the right place
and the staged installation works correctly.

Closes: xrootd#1768
amadio added a commit to amadio/xrootd that referenced this issue Nov 8, 2022
amadio added a commit to amadio/xrootd that referenced this issue Nov 9, 2022
amadio added a commit to amadio/xrootd that referenced this issue Nov 11, 2022
amadio added a commit to amadio/xrootd that referenced this issue Dec 1, 2022
amadio added a commit to amadio/xrootd that referenced this issue Dec 5, 2022
amadio added a commit to amadio/xrootd that referenced this issue Dec 5, 2022
@StefanBruens
Copy link

Using "--prefix" for DESTDIR is wrong, as pip may then write the DESTDIR into some generated files. The correct way to specify DESTDIR is by using "--root". E.g.:
--root "\$ENV{DESTDIR}" ${PIP_OPTIONS}

Using --root "" should have the same effect as omitting the parameter, i.e. this should also work when DESTDIR is not set.

@amadio amadio added this to the 5.6.0 milestone Feb 15, 2023
@amadio
Copy link
Member Author

amadio commented Feb 21, 2023

Unfortunately, using --root doesn't prevent pip from trying to uninstall stuff from /usr, which is what we want to prevent when doing staged installations (which are not installing into /usr, but into a staging area). This is described in pypa/pip#3063, which is also linked in the description.

epsftws build $ python3.10 -m pip install --root $PWD/install --prefix /usr bindings/python
Processing ./bindings/python
  Preparing metadata (setup.py) ... done
Building wheels for collected packages: xrootd
  Building wheel for xrootd (setup.py) ... done
  Created wheel for xrootd: filename=xrootd-5.5.3.post196-cp310-cp310-linux_x86_64.whl size=740187 sha256=6db2dd81ffc1f1b10d29e4aa4e299644e2c4d7084fa44302675b158c82094738
  Stored in directory: /tmp/pip-ephem-wheel-cache-y388wzs6/wheels/4f/bf/c3/60b496a0b3d3cc1bbe87053fb501e707645f2f8833a13d0c18
Successfully built xrootd
Installing collected packages: xrootd
  Attempting uninstall: xrootd
    Found existing installation: xrootd 5.5.3
    Uninstalling xrootd-5.5.3:
ERROR: Could not install packages due to an OSError: [Errno 13] Permission denied: '/usr/lib/python3.10/site-packages/XRootD/__init__.py'
Consider using the `--user` option or check the permissions.

@amadio amadio modified the milestones: 5.6, 6.0 Mar 3, 2023
@amadio amadio modified the milestones: 6.0, 5.6 May 30, 2023
amadio added a commit to amadio/xrootd that referenced this issue Jun 5, 2023
This is a rewrite of the packaging of the Python bindings. The new
packaging supports building the Python bindings both as part of a
standard CMake build, as well as against a previously installed version
of XRootD without the Python bindings. A new setup.py at the top level
has been created to replace the old one from packaging/wheel. It can
be used to drive the main CMake build using pip to create source and
binary distributions of XRootD.

Closes: xrootd#1768, xrootd#1807 xrootd#1833, xrootd#1844, xrootd#2001, xrootd#2002.
amadio added a commit to amadio/xrootd that referenced this issue Jun 5, 2023
This is a rewrite of the packaging of the Python bindings. The new
packaging supports building the Python bindings both as part of a
standard CMake build, as well as against a previously installed version
of XRootD without the Python bindings. A new setup.py at the top level
has been created to replace the old one from packaging/wheel. It can
be used to drive the main CMake build using pip to create source and
binary distributions of XRootD.

Closes: xrootd#1768, xrootd#1807 xrootd#1833, xrootd#1844, xrootd#2001, xrootd#2002.
amadio added a commit to amadio/xrootd that referenced this issue Jun 5, 2023
This is a rewrite of the packaging of the Python bindings. The new
packaging supports building the Python bindings both as part of a
standard CMake build, as well as against a previously installed version
of XRootD without the Python bindings. A new setup.py at the top level
has been created to replace the old one from packaging/wheel. It can
be used to drive the main CMake build using pip to create source and
binary distributions of XRootD.

Closes: xrootd#1768, xrootd#1807 xrootd#1833, xrootd#1844, xrootd#2001, xrootd#2002.
amadio added a commit to amadio/xrootd that referenced this issue Jun 6, 2023
This is a rewrite of the packaging of the Python bindings. The new
packaging supports building the Python bindings both as part of a
standard CMake build, as well as against a previously installed version
of XRootD without the Python bindings. A new setup.py at the top level
has been created to replace the old one from packaging/wheel. It can
be used to drive the main CMake build using pip to create source and
binary distributions of XRootD.

Closes: xrootd#1768, xrootd#1807 xrootd#1833, xrootd#1844, xrootd#2001, xrootd#2002.
amadio added a commit to amadio/xrootd that referenced this issue Jun 6, 2023
This is a rewrite of the packaging of the Python bindings. The new
packaging supports building the Python bindings both as part of a
standard CMake build, as well as against a previously installed version
of XRootD without the Python bindings. A new setup.py at the top level
has been created to replace the old one from packaging/wheel. It can
be used to drive the main CMake build using pip to create source and
binary distributions of XRootD.

Closes: xrootd#1768, xrootd#1807 xrootd#1833, xrootd#1844, xrootd#2001, xrootd#2002.
@amadio amadio closed this as completed in 2401509 Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants