Build with cmake 2.8.11 on Linux fails to install libraries into /usr/lib64

[mcepl]

I am trying to update Fedora tidy to 5.0.0 version and current (very experimental, unfinished, etc.) https://mcepl.fedorapeople.org/tmp/tidy-5.0.0-1.fc22.src.rpm fails because make install puts libraries (libtidy*) into /usr/lib/ instead of /usr/lib64/ directory. Whatever magical CMake variables I am setting nothing leads to the desired solution.
Using cmake 2.8.11 on RHEL-7.
Build log is build-5.0.0-1.fc22.txt

[mcepl]

Also, even though I have -DBUILD_TAB2SPACE:BOOL=ON I don’t see tab2space installed in /usr/bin/.

[geoffmcl]

@mcepl good to see someone looking after a Fedora build, install and packaging...

As you have found, and as your build log shows, lines 47-55, none of your magical Cmake variables do anything about this ;=()

The install sub-directory is controlled around Lines 168+. You would need to change the DESTINATION to lib64 if that is where you want it installed...

Now it would be possible to make this a user option, like say -

if (NOT LIB_INSTALL_DIR)
    set(LIB_INSTALL_DIR lib)
endif ()

Then you could add an option -DLIB_INSTALL_DIR:PATH=lib64. And likewise perhaps also a -DBIN_INSTALL_DIR=bin...

So then we could have -

    install(TARGETS ${name}
        RUNTIME DESTINATION ${BIN_INSTALL_DIR}
        ARCHIVE DESTINATION ${LIB_INSTALL_DIR}
        LIBRARY DESTINATION ${LIB_INSTALL_DIR}
        )    

Now this would then be relative to a -DCMAKE_INSTALL_PREFIX=/usr, which I note you do add...

And IIRC if the user gives absolute paths here, like -DLIB_INSTALL_DIR=/usr/lib64, then cmake will use that absolute path... need to experiment with that...

So this could be fixed, but we would be unlikely to push such a small cmake only fix back into release/5.0.0, making it release/5.0.1, especially since I hope we will have another release within a few months...

Another comments on reading your build log, is concerning adding -DNDEBUG to the compile. CMake will add this automatically if you set the -DCMAKE_BUILD_TYPE=Release... replacing 3 options with one...

Oops, but then I note you already have -DCMAKE_BUILD_TYPE:STRING=Debug! Why would you want any Debug code in a distribution package? Specifically in tidy the only unix debug code is quite a number of asserts, which compile to nothing is -DNDEBUG is added...

And note the -DBUILD_SHARED_LIBS:BOOL=ON defaults to ON so no need to add that...

I like your -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON option. This adds a lot of important information to the build log... I also use this all the time...

But that is getting away from the main issue here about the library install path...

So, for 5.0.0 building you will have to manually adjust that, and for the future, I will certainly look at adding the above install path user variables, like I have seen in several other cmake projects...

[mcepl]

As you have found, and as your build log shows, lines 47-55, none of your magical Cmake variables do anything about this ;=()

Oh well, c’est la vie

The install sub-directory is controlled around Lines 168+. You would need to change the DESTINATION to lib64 if that is where you want it installed...

Something like:

--- a/build/documentation/build_docs.sh
+++ b/build/documentation/build_docs.sh
@@ -7,7 +7,7 @@
 # documentation. Relative path is okay. You shouldn't have to change this
 # too often if your compiler always puts tidy in the same place.

-TIDY_PATH="../cmake/tidy5"         # Current directory.
+TIDY_PATH="../cmake/tidy"         # Current directory.

 TIDY_VERSION=`cat ../../version.txt`

--- a/test/testone.sh
+++ b/test/testone.sh
@@ -19,7 +19,7 @@ set +f
 TESTNO=$1
 EXPECTED=$2
 #TIDY=../bin/tidy
-TIDY=../build/cmake/tidy5
+TIDY=../build/cmake/tidy
 if [ ! -f "$TIDY" ]; then
    echo "$BN: Can NOT locate binary '$TIDY'!"
    echo "$BN: Fix me with the correct location of the binary to run."
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -154,6 +154,15 @@ install( FILES ${HFILES} DESTINATION inc
 ########################################
 # if user option still on
 if (BUILD_SHARED_LIB)
+
+    if (NOT LIB_INSTALL_DIR)
+        set(LIB_INSTALL_DIR lib)
+    endif ()
+
+    if (NOT BIN_INSTALL_DIR)
+        set(BIN_INSTALL_DIR bin)
+    endif ()
+
     set(name tidy-share)
     add_library ( ${name} SHARED ${CFILES} ${HFILES} ${LIBHFILES} )
     set_target_properties( ${name} PROPERTIES
@@ -166,11 +175,11 @@ if (BUILD_SHARED_LIB)
     set_target_properties( ${name} PROPERTIES
                                    COMPILE_FLAGS "-DBUILDING_SHARED_LIB" )
     install(TARGETS ${name}
-        RUNTIME DESTINATION bin
-        ARCHIVE DESTINATION lib
-        LIBRARY DESTINATION lib
-        )
-endif ()
+        RUNTIME DESTINATION ${BIN_INSTALL_DIR}
+        ARCHIVE DESTINATION ${LIB_INSTALL_DIR}
+        LIBRARY DESTINATION ${LIB_INSTALL_DIR}
+    )
+endif ()

 ##########################################################
 ### main executable - linked with STATIC library

Now this would then be relative to a -DCMAKE_INSTALL_PREFIX=/usr, which I note you do add...

And IIRC if the user gives absolute paths here, like -DLIB_INSTALL_DIR=/usr/lib64, then cmake will use that absolute path... need to experiment with that...

rpm automatically defines macro:

%__cmake \
        -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
        -DCMAKE_INSTALL_PREFIX:PATH=%{_prefix} \
        -DINCLUDE_INSTALL_DIR:PATH=%{_includedir} \
        -DLIB_INSTALL_DIR:PATH=%{_libdir} \
        -DSYSCONF_INSTALL_DIR:PATH=%{_sysconfdir} \
        -DSHARE_INSTALL_PREFIX:PATH=%{_datadir} \
%if "%{?_lib}" == "lib64"
        %{?_cmake_lib_suffix64} \
%endif

So this could be fixed, but we would be unlikely to push such a small cmake only fix back into release/5.0.0, making it release/5.0.1, especially since I hope we will have another release within a few months...

No problem, I can happily have this as a distro-specific patch for some time.

Oops, but then I note you already have -DCMAKE_BUILD_TYPE:STRING=Debug! Why would you want any Debug code in a distribution package? Specifically in tidy the only unix debug code is quite a number of asserts, which compile to nothing is -DNDEBUG is added...

That’s probably a mistake. With some other CMake build package, I had problems to get debugging symbols (to be later extracted into -debuginfo package).

I like your -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON option. This adds a lot of important information to the build log... I also use this all the time...

When building in the dedicated building server (e.g., http://koji.fedoraproject.org/koji/taskinfo?taskID=11822320) you have just limited means how to investigate problems so verbose logs are often all you have.

So, for 5.0.0 building you will have to manually adjust that, and for the future, I will certainly look at adding the above install path user variables, like I have seen in several other cmake projects...

Would the above patch had a chance of being accepted as a pull request?

[rdieter]

I'll submit a pull request shortly

[geoffmcl]

@mcepl your CMakeLists.txt patch looks fine... Assume you have tested it and can now build the rpm?

If it works, then for sure a PR would be accepted against the current 'master', and be there for the next release....

But you must remove the patch to build/documentation/build_docs.sh. That folder will longer exists, and I think this has been fixed the new documentation/build_docs.sh, if there is one...

@balthisar is presently working on this in the tidy_doc_cleanup branch, which, when completed, tested, will be merged to master...

Also test/testone.sh has been fixed in master some time ago...

In any case, I know it is extra work, but we prefer PRs be only about 1 issue at a time.

This is actually quite difficult in github. It collects all commits and puts them in the one PR. It seems the only (silly!) way around this is to put each fix in a new branch... extra work...

But for sure, let's have a flexible CMake install locations... and maybe we should also consider an ${INC_INSTALL_DIR} for the headers. Someone might like them installed to say include/tidy, etc...

Oops, just noted @rdieter added PR #300 which includes the static library... so this looks good also...
at least before we consider GNUInstallDirs... must experiment with this...

Concerning gcc debug symbols, IIRC -DENABLE_DEBUG_SYMBOLS:BOOL=TRUE will also add the -g option, need to check, but why add debug symbols only to strip them later? This is done very differently in WIN32.

Another thing noted in your log was that you run testall.sh. Nothing particularly wrong with that, but you do realise these are not any sort of unit testing in the usual sense. It is quite a collection of tests, about 228, that we have tagged as regression tests... and would have been run before we made that release... so do not expect distribution builders would need to run them...

This testing process is not yet well documented, but when a code change is done, a developer is expected to have run these tests before submitting a PR. To check for regressions... And if they forget I try to do it before the merge...

And in any case the testall.sh misses a final important part of the test, and that is to compare the results written to tmp with the testbase - a simple $ diff -ua testbase tmp.

But be aware there can be some differences which have to be accepted, like the different path separators between unix and windows... if we adjust testbase for one os, then there are differences in the other... we are still pondering what to do about this... so do not wildly publicise this until it is solved somehow... and better documented... however some improvements have been done since that 5.0.0 release...

On the other hand, you could add that last diff to your build log just to make sure the 3 or 4 presently expected difference are on our radar. See #266 - thanks...

[mcepl]

Concerning gcc debug symbols, IIRC -DENABLE_DEBUG_SYMBOLS:BOOL=TRUE will also add the -g option, need to check, but why add debug symbols only to strip them later? This is done very differently in WIN32.

That's the way it is done for all Linuxes … you build with the debugging symbols and then strip them and save to the separate file (packages -debuginfo in Fedora/RHEL case; see for example what's build for the older version of tidy in http://koji.fedoraproject.org/koji/buildinfo?buildID=662744 ). Normal user installs only small packages without symbols, but if the crash happens and they are needed for debugging, the -debuginfo package can be always installed (-debuginfo package for large packages like kernel or LibreOffice can have hundreds of megabytes). gdb and other debugging tools know how to use the information from the separately stored debugging files.

[mcepl]

On the other hand, you could add that last diff to your build log just to make sure the 3 or 4 presently expected difference are on our radar. See #266 - thanks...

OK, I can see that tests are not done yet (OTOH, don't underestimate niceness of running tests on weird architectures ... how many s390 with 31bit CPU you have?). :)

[geoffmcl]

@mcepl ok that is neat you offer both a release and a -debuginfo package to run with gdb. That would really help a user track down a bug, and provide a fix...

Maybe we should consider that for Ubuntu and Windows... bump the binary packages offered...

don't underestimate niceness of running tests on weird architectures

Well we don't, but what we did reject was using Travis-CI directly attached to the htacg tidy-html5 repo. I tried to encourage @vielmetti to continue this using his fork, and asked for more information on some of the test results to see what could be done... sadly he has chosen not to reply...

And like you have done with a tidy package for armv7hl, i686 and x86_64 we did want to develop a naming scheme, see bottom of #258, but it lapsed due to lack of feed back... need support to develop a cmake test to have something like tidy-$VERSION-$CPU-$BITNESS.deb

As you said, Oh well, c’est la vie ;=))

[mcepl]

Maybe we should consider that for Ubuntu and Windows... bump the binary packages offered...

This is not anything new. Enterprise distros (i.e., SUSE and Red Hat Linuxes) have done this for years. The idea is that you install clean packages on the production systems and only when crash happens, you can install -debuginfo packages and get clean tracebacks. Debian/Ubuntu has a bit different system (special -dbg packages), but that's not that flexible, IMHO.

[geoffmcl]

@mcepl distros have done this for years...

Absolutely, could not agree more... at least on the limited perception of a mainly "Windows" user, with a token linux system, for building, testing...

In windows it is a little more difficult, in that you need to also distribute a debug library, and including the separate *.dbf file, for the symbols... not easily done, but can be achieved... some projects have tried this with varying success...

However, due to the failure rate of those projects I have worked on, I do not recommend this path for tidy windows, but maybe there is a CPack option that is easily available for unix? Maybe there is a way to also store the debug version of a unix binary as well... That I would like to hear about... a PR or patch appreciated...

So that purely a windows difficulty has got nothing to do with what is possible using CMake CPack in unix. Maybe it is possible to also generate a -debuginfo version for distribution... That I would be interested to hear about... a PR or patch welcome...

But what has this to do with your subject? - install libraries into /usr/lib64 - this seems to has been adresseed, and I hope closed? Is this still closed?

OT: It certainly does seem we should expand the naming convention of a distribution binaries, that is for sure, and we seek ideas on how to do this... maybe add say a $DBG versions?... several other issues begin to address this... your comments in those would be much appreciated.