rscada / libmbus

Meter-bus library and utility programs
http://www.rscada.se/libmbus
BSD 3-Clause "New" or "Revised" License
217 stars 137 forks source link

build: add cmake support #151

Closed gocarlos closed 4 years ago

lategoodbye commented 4 years ago

Could you please explain why conan support is necessary? Also why all these changes to the sources?

gocarlos commented 4 years ago

Could you please explain why conan support is necessary? Also why all these changes to the sources?

furthermore I'll add a C++ RAII wrapper in the folder contrib, this is optional e.g. default off at compile time, this MR is WIP

gocarlos commented 4 years ago

lets add conan and wrapper in a next MR

lategoodbye commented 4 years ago
* conan support is here to allow others to consume this library with a language package manager.

I like to have this a separate commit.

* only change to the sources were moving `bin` to `example` (people tend today imho to expect a `include`, `test`, `src` and `example` folder) and eventually some automatic formatting (this repo does not have any style formatter ala clang-format)
  the move to `example` can be reverted of course

I'm not against this change, i only wanted to know why. The initial idea was a library, but from my understanding most of the user doesn't use the library to build an own application. They only use the example binaries.

gocarlos commented 4 years ago
* conan support is here to allow others to consume this library with a language package manager.

I like to have this a separate commit.

* only change to the sources were moving `bin` to `example` (people tend today imho to expect a `include`, `test`, `src` and `example` folder) and eventually some automatic formatting (this repo does not have any style formatter ala clang-format)
  the move to `example` can be reverted of course

I'm not against this change, i only wanted to know why. The initial idea was a library, but from my understanding most of the user doesn't use the library to build an own application. They only use the example binaries.

I’ll rebase and change the patch/commit message accordingly

Apollon77 commented 4 years ago

@lategoodbye

The initial idea was a library, but from my understanding most of the user doesn't use the library to build an own application. They only use the example binaries.

Are you really sure? Yes for end-users this might be true. But your library is included as lib in many projects! Not only my nodejs library is based on it, also volkzaehler project and others ... soI think it is more often used as library then as binaries ... ;-)

lategoodbye commented 4 years ago

@lategoodbye

The initial idea was a library, but from my understanding most of the user doesn't use the library to build an own application. They only use the example binaries.

Are you really sure? Yes for end-users this might be true. But your library is included as lib in many projects! Not only my nodejs library is based on it, also volkzaehler project and others ... soI think it is more often used as library then as binaries ... ;-)

My statement based on the Github issues, since i don't have statistics about the libmbus usage. Maybe you have a better overview about the usage. I only want to say that the "example" binaries are an important part of this project.

gocarlos commented 4 years ago

I only want to say that the "example" binaries are an important part of this project.

I would rename bin to examples as I think those are examples...

but this can potential be done in a next MR, @lategoodbye is here anything else you need to have to get this merged?

lategoodbye commented 4 years ago

I only want to say that the "example" binaries are an important part of this project.

I would rename bin to examples as I think those are examples...

And that's fine.

lategoodbye commented 4 years ago

Since you introduce cmake, please try to take care of the other stuff like deb / RPM spec. Sorry, i'm not that cmake expert but it would be great to make maintaining easier.

Btw i suggest to use the github URL instead of the rscada because we have the newer releases.

gocarlos commented 4 years ago

Since you introduce cmake, please try to take care of the other stuff like deb / RPM spec. Sorry, i'm not that cmake expert but it would be great to make maintaining easier.

Btw i suggest to use the github URL instead of the rscada because we have the newer releases.

this should pretty straight forward, can take on that.... though I think that if we are going this path (my goal was originally to just add a simple cmake build script to make integration with conan/cmake easy), then it does not make sense to keep the other build system e.g. we should then also remove it in the future

lategoodbye commented 4 years ago

In the long run, i don't want to maintain two build systems (except of Windows). So i appreciate a proper cut, if it's possible.

Apollon77 commented 4 years ago

I hope I get my windows fork working with all these changes ;-) might be a challenge ... we will see

gocarlos commented 4 years ago

I hope I get my windows fork working with all these changes ;-) might be a challenge ... we will see

almost nothing changed -> added cmake scripts and config.h which is then configured by cmake... your fork is mostly doing changes to the source code itself e.g. serial abstraction layer ?

Apollon77 commented 4 years ago

@gocarlos basically as in https://github.com/rscada/libmbus/pull/133/files ;-)

Apollon77 commented 4 years ago

I don't found time till now to adjust to @lategoodbye 's wishes to get progress

gocarlos commented 4 years ago

@lategoodbye could you review again? the docker images are here to create the rpm and deb files one is using another system e.g. mac or windows

gocarlos commented 4 years ago
  • Is it ok to remove the autotools build system?
  • the cmake script should also generate .pc files.

thanks for the feedback.

I think that if we have all the capabilities of the old build system, why keep it and maintain it as @lategoodbye said?

gocarlos commented 4 years ago

.pc files are now installed again and configured over cmake automatically

could be merged from my side

gocarlos commented 4 years ago

eventually also squashed into 1 commit

gocarlos commented 4 years ago

Ping

gocarlos commented 4 years ago

review addressed

lategoodbye commented 4 years ago

Thanks. But on Ubuntu 18.04 which uses cmake 3.10.2, it still failes:

$ ./build.sh 
-- The CXX compiler identification is GNU 7.5.0
-- The C compiler identification is GNU 7.5.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- CMAKE_BUILD_TYPE empty setting to Debug
-- Building with code coverage...
-- Looking for dlfcn.h
-- Looking for dlfcn.h - found
-- Looking for inttypes.h
-- Looking for inttypes.h - found
-- Looking for memory.h
-- Looking for memory.h - found
-- Looking for stdlib.h
-- Looking for stdlib.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for strings.h
-- Looking for strings.h - found
-- Looking for string.h
-- Looking for string.h - found
-- Looking for sys/stat.h
-- Looking for sys/stat.h - found
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for unistd.h
-- Looking for unistd.h - found
-- building examples
-- building tests
-- Configuring done
-- Generating done
-- Build files have been written to: /home/stefan/libmbus/_build
Unknown argument -j
Usage: cmake --build <dir> [options] [-- [native-options]]
Options:
  <dir>          = Project binary directory to be built.
  --target <tgt> = Build <tgt> instead of default targets.
                   May only be specified once.
  --config <cfg> = For multi-configuration tools, choose <cfg>.
  --clean-first  = Build target 'clean' first, then build.
                   (To clean only, use --target 'clean'.)
  --use-stderr   = Ignored.  Behavior is default in CMake >= 3.0.
  --             = Pass remaining options to the native tool.
gocarlos commented 4 years ago

Thanks. But on Ubuntu 18.04 which uses cmake 3.10.2, it still failes:

$ ./build.sh 
-- The CXX compiler identification is GNU 7.5.0
-- The C compiler identification is GNU 7.5.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- CMAKE_BUILD_TYPE empty setting to Debug
-- Building with code coverage...
-- Looking for dlfcn.h
-- Looking for dlfcn.h - found
-- Looking for inttypes.h
-- Looking for inttypes.h - found
-- Looking for memory.h
-- Looking for memory.h - found
-- Looking for stdlib.h
-- Looking for stdlib.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for strings.h
-- Looking for strings.h - found
-- Looking for string.h
-- Looking for string.h - found
-- Looking for sys/stat.h
-- Looking for sys/stat.h - found
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for unistd.h
-- Looking for unistd.h - found
-- building examples
-- building tests
-- Configuring done
-- Generating done
-- Build files have been written to: /home/stefan/libmbus/_build
Unknown argument -j
Usage: cmake --build <dir> [options] [-- [native-options]]
Options:
  <dir>          = Project binary directory to be built.
  --target <tgt> = Build <tgt> instead of default targets.
                   May only be specified once.
  --config <cfg> = For multi-configuration tools, choose <cfg>.
  --clean-first  = Build target 'clean' first, then build.
                   (To clean only, use --target 'clean'.)
  --use-stderr   = Ignored.  Behavior is default in CMake >= 3.0.
  --             = Pass remaining options to the native tool.

you can just remove the last -j this is only supported in newer cmake version -> this triggers multiprocess/multithread compilation

gocarlos commented 4 years ago

Thanks. But on Ubuntu 18.04 which uses cmake 3.10.2, it still failes:

$ ./build.sh 
-- The CXX compiler identification is GNU 7.5.0
-- The C compiler identification is GNU 7.5.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- CMAKE_BUILD_TYPE empty setting to Debug
-- Building with code coverage...
-- Looking for dlfcn.h
-- Looking for dlfcn.h - found
-- Looking for inttypes.h
-- Looking for inttypes.h - found
-- Looking for memory.h
-- Looking for memory.h - found
-- Looking for stdlib.h
-- Looking for stdlib.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for strings.h
-- Looking for strings.h - found
-- Looking for string.h
-- Looking for string.h - found
-- Looking for sys/stat.h
-- Looking for sys/stat.h - found
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for unistd.h
-- Looking for unistd.h - found
-- building examples
-- building tests
-- Configuring done
-- Generating done
-- Build files have been written to: /home/stefan/libmbus/_build
Unknown argument -j
Usage: cmake --build <dir> [options] [-- [native-options]]
Options:
  <dir>          = Project binary directory to be built.
  --target <tgt> = Build <tgt> instead of default targets.
                   May only be specified once.
  --config <cfg> = For multi-configuration tools, choose <cfg>.
  --clean-first  = Build target 'clean' first, then build.
                   (To clean only, use --target 'clean'.)
  --use-stderr   = Ignored.  Behavior is default in CMake >= 3.0.
  --             = Pass remaining options to the native tool.

you can just remove the last -j this is only supported in newer cmake version -> this triggers multiprocess/multithread compilation

done

lategoodbye commented 4 years ago

Sorry to bother you again, but in the original deb/rpm definition there were two packages non-devel and devel. Is it possible to keep this?

I did the following:

rm -rf _build
mkdir _build
cd _build
cmake .. -DLIBMBUS_PACKAGE_DEB=ON
cpack ..
gocarlos commented 4 years ago

Sorry to bother you again, but in the original deb/rpm definition there were two packages non-devel and devel. Is it possible to keep this?

I did the following:

rm -rf _build
mkdir _build
cd _build
cmake .. -DLIBMBUS_PACKAGE_DEB=ON
cpack ..

have no idea what is devel and non-devel packages, I'll have a look on this...

lategoodbye commented 4 years ago

I think i've found another issue. If i run ./build the test binaries are build, but not copied into the test directory. This is necessary for the generate-xml.sh script.

gocarlos commented 4 years ago

I think i've found another issue. If i run ./build the test binaries are build, but not copied into the test directory. This is necessary for the generate-xml.sh script.

actually I changed the generate-xml.sh in the next MR to take in the location of the binaries... copying the binaries into the source tree is a bad practice. All artifacts should be in the build directory

gocarlos commented 4 years ago

already 63 comments for a simple cmake script 🥇 👯 😄

madebr commented 4 years ago

already 63 comments for a simple cmake script 🥇 👯 😄

Let's stop at 69 :3 😄

gocarlos commented 4 years ago

I think i've found another issue. If i run ./build the test binaries are build, but not copied into the test directory. This is necessary for the generate-xml.sh script.

actually I changed the generate-xml.sh in the next MR to take in the location of the binaries... copying the binaries into the source tree is a bad practice. All artifacts should be in the build directory

done

gocarlos commented 4 years ago

@lategoodbye still something missing?

lategoodbye commented 4 years ago

I think i've found another issue. If i run ./build the test binaries are build, but not copied into the test directory. This is necessary for the generate-xml.sh script.

actually I changed the generate-xml.sh in the next MR to take in the location of the binaries... copying the binaries into the source tree is a bad practice. All artifacts should be in the build directory

done

Sorry, i didn't noticed that you are finished with the pull request.

I really hate scripts which are impractical. Could you please make the second parameter optional and use ./_build/bin/mbus_parse_hex as default? This would make it behave as before.

gocarlos commented 4 years ago

I think i've found another issue. If i run ./build the test binaries are build, but not copied into the test directory. This is necessary for the generate-xml.sh script.

actually I changed the generate-xml.sh in the next MR to take in the location of the binaries... copying the binaries into the source tree is a bad practice. All artifacts should be in the build directory

done

Sorry, i didn't noticed that you are finished with the pull request.

I really hate scripts which are impractical. Could you please make the second parameter optional and use ./_build/bin/mbus_parse_hex as default? This would make it behave as before.

done

lategoodbye commented 4 years ago

Thanks

lategoodbye commented 4 years ago

@gocarlos @madebr

Unfortunately i found another issue. Now the libmbus is installed under the wrong name: liblibmbus.so if i call make install from the build directory.

Sorry, for not noticing this before. Can anyone of you give me an hint or send a pull request to fix this?

Thanks

gocarlos commented 4 years ago

Add_library in cmake is using the name libmbus, as linux uses lib prefix it produces liblibmbus

set_target_properties(${PROJECT_NAME} PROPERTIES PREFIX "")

lategoodbye commented 4 years ago

Thanks again. I pushed 1e25cf10964237384762deac7b24749affaef42d

lategoodbye commented 3 years ago

@gocarlos In #174 i noticed that some build artifacts like libmbus.a libmbus.la are now missing. Is it possible to fix this?