DGtal-team / DGtal

Digital Geometry Tools and Algorithm Library
https://dgtal.org
GNU Lesser General Public License v3.0
370 stars 115 forks source link

make install DESTDIR="<path>" ignored by some files #1229

Closed copyme closed 7 years ago

copyme commented 7 years ago

Since version 0.9.2 there is a problem with installing DGtal to a directory pointed by DESTDIR. For example one can notice that there is an attempt to copy into /usr/include/DGtal/topology/tables/NeighborhoodTables.h.tmp, instead to a directory pointed by DESTDIR.

The problem seems to be related to the look up tables introduced in 0.9.2. The issue was extracted from #1207.

dcoeurjo commented 7 years ago

I think the best way to define an install dir is to do it when running cmake (using "CMAKE_INSTALL_DIR" I think)

dcoeurjo commented 7 years ago

I've never used DESTDIR at make level

copyme commented 7 years ago

@dcoeurjo It will not work when you want to build a Linux package. With CMake you will set a real destination but you need DESDIR for make to build up everything.

dcoeurjo commented 7 years ago

I would say that cmake is in charge of the build, install and package construction... by tweaking Destdir at the make level, I'm not sure if it is really appropriate. In other words: I've really no idea how to control the "make" Generator without using cmake variables ;)

copyme commented 7 years ago

@dcoeurjo It is and is even documented in the CMake documentation: https://cmake.org/cmake/help/v3.0/variable/CMAKE_INSTALL_PREFIX.html It is also a default way used by spec files of rpmbuild.

dcoeurjo commented 7 years ago

Ok I see. My bad. And I see the issue with the neighborhood tables that are handled at cmake level (with non trivial rules). I don't know to handle that at make one. :sad:

copyme commented 7 years ago

I've searched the cmake bug tracker and I found some 5 years old bug which seems to be related to the problem. I added a new comment related to our problem there. Stay tuned.

copyme commented 7 years ago

From Ben Boeckle (@ben.boeckel, cmake developer): "@copyme That is an issue in DGtal itself. On this line, the code needs to use $ENV{DESTDIR} manually. CMake adds DESTDIR to its logic, but manual logic needs to incorporate it manually. The docs may be deficient here."

The link to the topic on gitlab.kitwere.com/cmake: https://gitlab.kitware.com/cmake/cmake/issues/11704#note_212469

copyme commented 7 years ago

@phcerdan Could you have a look at this issue, please?

phcerdan commented 7 years ago

@copyme thanks for this, I had no idea about DESTDIR. I'd definetely have a look to it, but I have no reliable access to computer/internet until the 4th.

copyme commented 7 years ago

@phcerdan Thanks and no problem there is no time pressure. Happy new year, btw!

phcerdan commented 7 years ago

@copyme thanks! Same for you! :)

phcerdan commented 7 years ago

@copyme tested locally and it works following @ben.boeckel comment from the link you provided https://gitlab.kitware.com/cmake/cmake/issues/11704#note_212469.

Completely other topic but related with packaging: the tables are a bit too big when unzipped (200MB). The solution to this can be: A) smarter way to represent many booleans in a file, other than single char with 0,1. B) using serialization (binary object) from boost. Or optional installation/unzipping of tables.

dcoeurjo commented 7 years ago

@phcerdan For the second comment, if #1228 is merged, we could use also zlib to efficiently compress the tables. :)

phcerdan commented 7 years ago

@dcoeurjo that would be mint! I am using cmake built-in compressor/decompresor with the tables. And compressing is not a problem, the files are quite small. The problem is having to decompress them to a big plain text file at installation/build time for them to be ready to use.

So, with your approach in #1228 having zlib and boost::streams. Would we read the tables directly from the compressed file?

dcoeurjo commented 7 years ago

Yes that's the idea: Keep compressed tables on disk and uncompress them in memory when read.

Let's wait for more feedbacks on #1228. The main drawback is that we have a new mandatory dependency for DGtal (zlib)

phcerdan commented 7 years ago

It seems a reasonable small dependency though. I will keep an eye to #1228 then. Thanks!

copyme commented 7 years ago

@phcerdan Thanks for fixing this! I will try to build Linux packages this Friday and give you my feedback. But, yes I agree that keeping these LUTs compressed sounds like a great idea. Having up to 200MB in the root partition just for LUTS is not what I would personally accept -- I tend to keep very small root partition on my machines.

phcerdan commented 7 years ago

@copyme I agree, I will test the approach proposed by @dcoeurjo with zlib when #1228 is merged.

copyme commented 7 years ago

@phcerdan There is still something wrong (maybe on my side right now). I just checked NeighborhoodTables.h and I found inside wrong paths: "/home/plutak/rpmbuild/BUILDROOT/dgtal-0.9.3-0.x86_64/usr/include/DGtal/topology/tables/simplicity_table26_6.zlib", etc. I'm working on this.

The correct path should be: "/usr/include/DGtal/topology/tables/simplicity_table26_6.zlib"

copyme commented 7 years ago

@dcoeurjo Can we re-open, please? I cannot do this.

copyme commented 7 years ago

@phcerdan I think I will just create a patch for building system. No need to change anything in DGtal.

copyme commented 7 years ago

@phcerdan OK I built up packages. No need to change anything. @dcoeurjo we can keep this issue closed. Thanks!

phcerdan commented 7 years ago

That's my fault. DESTDIR should not appear on the file.

With zlib all the process of installing can be simplified. Hopefully, PR #1238 will fix this, it creates a temporary file and let cmake handle DESTDIR as it does with other files.