iamantony / qtcsv

Library for reading and writing csv-files in Qt.
MIT License
265 stars 141 forks source link

portable cmake #31

Closed cguentherTUChemnitz closed 7 years ago

cguentherTUChemnitz commented 7 years ago

Hi there,

i will use this ticket to comment some strategy decisions for your cmake implementation. With modern cmake you are not only able to handle your build environment, also others are able to integrate your project directly into their project structure. That is what i do currently and why i discuss a lot of your cmake efforts :)

A good starting point is for example this tutorial: https://rix0r.nl/blog/2015/08/13/cmake-guide/

Make QtCreator list all project files

Also a lot of IDEs (e.g. the QtCreator) are able to handle cmake projects directly. So no more qmake is then necessary. One little hack, which does not influence other stuff, is necessary to lets the IDE display all your project files. QtCreator displays only those, which are referenced by the build targets. To achieve this you can add this statement to your cmake files:

file(GLOB_RECURSE ALL_FILES "*")
add_custom_target(show_all_files_in_${PROJECT_NAME} SOURCES ${ALL_FILES})

Don't modify the cmake default behavior

When other guys include your project into their project structure, they expect default cmake behavior. So do not alter this. The "modern" part of the cmake tutorials are to set as most as possible configuration only for your target. This ensures, that no other targets are influenced by global cmake variables.

In-source and out-of-source builds

Please do not modify the

LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}

Cmake create build folder structures in cooperation with the used IDE. If you modify those pathes, it is possible that the IDE can't find the libs after building. I think you are not aware of one cmake concept. You can build where you want. So you can decide if you do an in-source build (putting the temp files and binaries into the source folders) or and out-of-source build (placing the files elsewhere, what is what the most IDEs do, to keep the source folder clean, like QtCreator). With your modification of the LIBRARY_OUTPUT_DIRECTORY you enforce the in-source build (for the lib placement), even when the IDE tries to do it otherwise. The build directory used by cmake is the dir, where you call the cmake statement. So when you want to do an in-source build, you only need to call cd qtcsv cmake .

cmake automatic compiler flags

Cmake is highly platform, compiler and toolchain independed. So you should try to set at least as possible flags, which depend either on specific compiler, platforms or toolchains. The most of your explicitly defined flags, are handled by cmake automatically.

  1. position independed code (-fPIC for gcc compiler) https://cmake.org/cmake/help/v3.0/command/add_library.html "For SHARED and MODULE libraries the POSITION_INDEPENDENT_CODE target property is set to ON automatically." So you don't need to set this flag. CMake does this for your at the moment, when you request to build a shared lib. A great benefit of such an automation is, that cmake also selects the correct flags for other compilers etc.
  2. compiler flags implicitly defined by CMAKE_BUILD_TYPE CMake provides a stock of build types. Those can be defined by calling cmake like this: cmake . -DCMAKE_BUILD_TYPE=RELEASE. This is the same what the IDEs do during their configuration. When you start for example QtCreator opening a Cmake project, it configures different out-of-source build folder with different CMAKE_BUILD_TYPE. The build types also switch automatically the compiler flags. This blog shows by example that the compiler flags for Release and Debug builds are the same, what explicitly set. https://ecrafter.wordpress.com/2012/04/24/cmake-tutorial-part-2/

    CMAKE_CXX_FLAGS_DEBUG is "-g" CMAKE_CXX_FLAGS_RELEASE is "-O3 -NDEBUG"

Summery - action points

I suggest to:

  1. delete the explicit compiler flags based on the build type: https://github.com/iamantony/qtcsv/blob/dev/CMakeLists.txt#L42-L51 Call cmake with -DCMAKE_BUILD_TYPE=[DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL]instead.

  2. ensure library placement handled by cmake Delete the LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} statements. https://github.com/iamantony/qtcsv/blob/dev/CMakeLists.txt#L66 https://github.com/iamantony/qtcsv/blob/dev/CMakeLists.txt#L79 Call cmake in your source folder for in-source builds instead.

  3. Optionally add a not used target containing all files to enforce some IDEs like QtCreator to show all files.

    add_custom_target(show_all_files_in_${PROJECT_NAME} SOURCES ${ALL_FILES})
iamantony commented 7 years ago

Hi. Good idea) Reading tutorial...

cguentherTUChemnitz commented 7 years ago

So, i updated my list of thoughts :)

iamantony commented 7 years ago

Very strong arguments) I'm totally agree with you. Please see latest commits in dev branch.

cguentherTUChemnitz commented 7 years ago

Very nice and thank you for your collaboration. I will check early next week, if this state is directly usable in our project. It looks good so far.

iamantony commented 7 years ago

I tried to unify build process with qmake and cmake, but not all went smoothly.

I edited .pro files so it became possible to build project into separate "build" folder. After successful build user will get this files in the build folder: - /build -- /data -- libqtcsv.so -- qtcsv_tests

But if I build project in "build" folder using cmake, I get this structure: - /build -- libqtqsv.so -- /tests --- /data --- qtscv_tests

How can we set up cmake so "data" folder and "qtcsv_tests" binary would appear not in "./build/tests" but in the "./build" directory? Or this is not a good idea and I should change qmake build process instead?

schulmar commented 7 years ago

You could force the build output of CMake into the QMake format but CMake does this for a reason: When you have source files with the same name in different directories, their object filenames will clash when they are put in the same directory. This has happened to me several times and is really annoying in QMake because you then have to change the colliding names.

There is an option for Qt5 QMake (CONFIG+=object_parallel_to_source) to mimic the source folder structure in the build directory but I remember having problems with that when working under Windows.

A toplevel subdirs project might also work.

cguentherTUChemnitz commented 7 years ago

Thanks @schulmar, for answering the most current question. The dev branch changes looks fine to me.

schulmar commented 7 years ago

I just noticed, that object_parallel_to_source would be of no help here, because it only fixes the problem with .o files within one project (one .pro file) whereas you want to define the output structure of two separate projects.

The proposed subdirs project would result in an extra tests subfolder in the build folder as it requires an extra project file in the sources/ subdirectory which would result in a sources/ subdirectory within the build folder as well. It would also add the tests to the default build.

The question is: what kind of layout would you prefer? What are its dis-/advantages?

iamantony commented 7 years ago

"Old" qmake build process that I used, now seems to me not so flexible as cmake build process. That is why I decided to use build directory layout which cmake generates.

I updated build instructions in Readme file (and also appveyor.yml and .travis.yml). Could you review it, please? Maybe I did it too complicated?

schulmar commented 7 years ago

I noticed following things:

The scripts look okay, although appveyor does not contain cmake. Copying the library into the test folder for QMake looks a bit sketchy (and might be avoidable as I explained above).

iamantony commented 7 years ago

@schulmar

  1. Fixed in README.md and .travis.yml
  2. Well, I hope it is not big problem too) Maybe @cguentherTUChemnitz could help with it...
  3. Yeah, I did not add windows builds with cmake to appveyor yet. I googled but could not find appropriate solution for creation of additional cmake-builds. Does anyone have any examples?
schulmar commented 7 years ago
  1. Fixed in README.md and .travis.yml

I am not sure about QMake on Windows (can't test), does it find the library during linktime?

  1. Yeah, I did not add windows builds with cmake to appveyor yet. I googled but could not find appropriate solution for creation of additional cmake-builds. Does anyone have any examples?

I think it is not made for using several build systems. You already solved that in .travis.yml by repeating the whole configuration matrix. I find this somewhat dissatisfactory as it introduces much repetition. I am not sure if this is a viable path but I would try to use one of the orthogonal properties that span the matrix, like the configuration (currently not used, instead of Release/Debug it could be CMake/QMake) in appveyor. However, I have little experience with both appveyor and travis, so these are just ideas.

cguentherTUChemnitz commented 7 years ago

Okey, i created a PR https://github.com/iamantony/qtcsv/pull/35 based on this blog post for cmake shared lib symlink chains: http://pusling.com/blog/?p=352

The desired behavior should be: libbar.so => libbar.so.0 => libbar.so.0.0.0 instead of (currently): libbar.so => libbar.so.0.0 => libbar.so.0.0.0

iamantony commented 7 years ago

Finally decided to merge dev into master. If there are no mistakes or new suggestions, then I will create a new release (v 1.4) soon.