Packaging issue for Python bindings with CMake: $DESTDIR not honored #72

Closed
zorun opened this Issue May 31, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@zorun
Contributor

zorun commented May 31, 2016

CMake supports the DESTDIR environment variable, which is useful when packaging: it allows to install all files under a given directory instead of /. A typical packaging script for Archlinux works like this:

mkdir build
cd build
cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_BUILD_TYPE=Release
make
make DESTDIR="$pkgdir" install
# Then produce a tarball from $pkgdir

Unfortunately, the python bindings for opendht try to install into /, without taking DESTDIR into account.

One possibility is the following RFC patch: https://gist.github.com/zorun/5d2495ef5547e6df3a5ede68cc97dea1 but it would need to test if DESTDIR is defined in the environment.

Another possibility is to allow disabling the installation of python bindings. Most Archlinux packages install python bindings without the help of the build system, using something like:

python setup.py install --prefix=/usr --root="$pkgdir" --optimize=1
@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 May 31, 2016

Contributor

Another possibility is to allow disabling the installation of python bindings.

You actually can disable the python build by doing to the following:

cmake -DOPENDHT_PYTHON=off # ...

However, as you pointed out, the python install should take the DESTDIR variable into account. I'll make a fix based on your patch that checks the environnement variable.

Contributor

sim590 commented May 31, 2016

Another possibility is to allow disabling the installation of python bindings.

You actually can disable the python build by doing to the following:

cmake -DOPENDHT_PYTHON=off # ...

However, as you pointed out, the python install should take the DESTDIR variable into account. I'll make a fix based on your patch that checks the environnement variable.

@sim590 sim590 added the bug label May 31, 2016

@sim590 sim590 self-assigned this May 31, 2016

@zorun

This comment has been minimized.

Show comment
Hide comment
@zorun

zorun May 31, 2016

Contributor

On Tue, May 31, 2016 at 09:54:10AM -0700, Simon Désaulniers wrote:

Another possibility is to allow disabling the installation of python bindings.

You actually can disable the python build by doing to the following:

cmake -DOPENDHT_PYTHON=off # ...

Well, I do want to build the python bindings, that's the point :)

What I meant is that we could tell CMake not to install the python files
when running make install. But I agree it looks more complicated than
the other solution.

However, as you pointed out, the python install should take the DESTDIR variable into account. I'll make a fix based on your patch that checks the environnement variable.

Ok, thanks!

Contributor

zorun commented May 31, 2016

On Tue, May 31, 2016 at 09:54:10AM -0700, Simon Désaulniers wrote:

Another possibility is to allow disabling the installation of python bindings.

You actually can disable the python build by doing to the following:

cmake -DOPENDHT_PYTHON=off # ...

Well, I do want to build the python bindings, that's the point :)

What I meant is that we could tell CMake not to install the python files
when running make install. But I agree it looks more complicated than
the other solution.

However, as you pointed out, the python install should take the DESTDIR variable into account. I'll make a fix based on your patch that checks the environnement variable.

Ok, thanks!

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 May 31, 2016

Contributor

If you do not want to install the bindings, but still want to build them, I guess you could work around by doing this:

$ cmake -DOPENDHT_PYTHON=on ..
$ make
$ cmake -DOPENDHT_PYTHON=off
$ sudo make install
Contributor

sim590 commented May 31, 2016

If you do not want to install the bindings, but still want to build them, I guess you could work around by doing this:

$ cmake -DOPENDHT_PYTHON=on ..
$ make
$ cmake -DOPENDHT_PYTHON=off
$ sudo make install
@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 2, 2016

Contributor

@zorun: I've tried your patch, but it doesn't work since $ENV{VAR} expands the environnement variable à CMake runtime. So, you have to actually do this:

install(CODE "execute_process(COMMAND python3 setup.py install --root=\$DESTDIR/ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})")

Notice:

  • the escaped $ and not braces surrounding the variable name. This let CMake interpret this as a simple string during CMake variable expansion and it will be expanded in Make runtime. However, I've noticed DESTDIR must be passed a full path, not relative path or else it will install relatively from the CMake setup.py generated file location. This may cause problems. The only way I see I can fix this is if I can catch the value passed to --root inside the python setup.py.in file and easily set the value to full path.
  • the / following the word DESTDIR. If $DESTDIR is not set, it will default to / which is expected behavior.
Contributor

sim590 commented Jun 2, 2016

@zorun: I've tried your patch, but it doesn't work since $ENV{VAR} expands the environnement variable à CMake runtime. So, you have to actually do this:

install(CODE "execute_process(COMMAND python3 setup.py install --root=\$DESTDIR/ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})")

Notice:

  • the escaped $ and not braces surrounding the variable name. This let CMake interpret this as a simple string during CMake variable expansion and it will be expanded in Make runtime. However, I've noticed DESTDIR must be passed a full path, not relative path or else it will install relatively from the CMake setup.py generated file location. This may cause problems. The only way I see I can fix this is if I can catch the value passed to --root inside the python setup.py.in file and easily set the value to full path.
  • the / following the word DESTDIR. If $DESTDIR is not set, it will default to / which is expected behavior.
@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 2, 2016

Contributor

About the absolute path issue. I guess I could also wrap setup.py install script with a shell script and check if path is not absolute. In that case, I would prefix it with the ${CMAKE_CURRENT_BINARY_DIR} variable before calling setup.py...

Contributor

sim590 commented Jun 2, 2016

About the absolute path issue. I guess I could also wrap setup.py install script with a shell script and check if path is not absolute. In that case, I would prefix it with the ${CMAKE_CURRENT_BINARY_DIR} variable before calling setup.py...

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 2, 2016

Contributor

Well. After playing alot with this, I didn't find a clean way to use the environment variable set during the make call... The trick I found earlier is broken if the environment variable is not set. I'll try further to find a solution later. In the mean time, the work around I stated above does the trick.

Contributor

sim590 commented Jun 2, 2016

Well. After playing alot with this, I didn't find a clean way to use the environment variable set during the make call... The trick I found earlier is broken if the environment variable is not set. I'll try further to find a solution later. In the mean time, the work around I stated above does the trick.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 2, 2016

Contributor

I have found a solution (#74) for CMake and autotools. It was really easier on Autotools' side. I've picked the solution for CMake here. Notice the escaped $. In my understanding, this is skipping expansion at the first CMake interpretting phase and passing along the symbol to interpret during the make stage.

Contributor

sim590 commented Jun 2, 2016

I have found a solution (#74) for CMake and autotools. It was really easier on Autotools' side. I've picked the solution for CMake here. Notice the escaped $. In my understanding, this is skipping expansion at the first CMake interpretting phase and passing along the symbol to interpret during the make stage.

@sim590 sim590 closed this Jun 2, 2016

@zorun

This comment has been minimized.

Show comment
Hide comment
@zorun

zorun Jun 2, 2016

Contributor

Great, it works, thanks :)

Contributor

zorun commented Jun 2, 2016

Great, it works, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment