build: add cmake support #151

gocarlos · 2020-03-17T13:00:42Z

No description provided.

lategoodbye · 2020-03-17T17:30:50Z

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

gocarlos · 2020-03-17T18:42:03Z

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

conan support is here to allow others to consume this library with a language package manager.
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

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 · 2020-03-17T20:26:03Z

lets add conan and wrapper in a next MR

mbus/mbus-protocol.h

lategoodbye · 2020-03-18T19:32:18Z

* 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 · 2020-03-18T19:40:03Z

* 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 add this lib directly to conan center as soon as cmake is merged (github release would be great after the merge as those package managers tend to use git tags)
ok i did not know that people only use the binaries, we just use them as a how to, e.g. example and link this lib to our existing binaries...

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

Apollon77 · 2020-03-18T21:29:34Z

@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 ... ;-)

CMakeLists.txt

lategoodbye · 2020-03-21T09:30:19Z

@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 · 2020-03-21T09:32:15Z

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 · 2020-03-21T09:39:38Z

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 · 2020-03-21T09:44:53Z

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 · 2020-03-21T10:00:35Z

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 · 2020-03-21T10:27:37Z

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 · 2020-03-21T23:02:22Z

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

gocarlos · 2020-03-22T08:09:35Z

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 · 2020-03-22T22:01:11Z

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

Apollon77 · 2020-03-22T22:01:55Z

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

gocarlos · 2020-03-23T12:24:41Z

@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 · 2020-03-31T19:28:23Z

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

Co-Authored-By: Anonymous Maarten <[email protected]>

CMakeLists.txt

gocarlos · 2020-04-01T13:55:41Z

already 63 comments for a simple cmake script 🥇 👯 😄

madebr · 2020-04-01T14:07:05Z

already 63 comments for a simple cmake script 🥇 👯 😄

Let's stop at 69 :3 😄

CMakeLists.txt

Co-Authored-By: Anonymous Maarten <[email protected]>

gocarlos · 2020-04-14T09:01:52Z

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 · 2020-04-22T15:53:15Z

@lategoodbye still something missing?

lategoodbye · 2020-04-23T20:11:49Z

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 · 2020-04-24T07:55:41Z

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 · 2020-04-24T18:18:26Z

Thanks

lategoodbye · 2020-06-27T12:59:39Z

@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 · 2020-06-27T13:57:17Z

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 · 2020-06-27T14:28:17Z

Thanks again. I pushed 1e25cf1

lategoodbye · 2020-07-14T10:44:13Z

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

gocarlos changed the title ~~feat: add cmake and conan support~~ build: add cmake and conan support Mar 17, 2020

gocarlos changed the title ~~build: add cmake and conan support~~ [WIP] build: add cmake and conan support Mar 17, 2020

gocarlos force-pushed the feat--add-cmake-support branch from 7070f19 to 5dcaeef Compare March 17, 2020 20:25

gocarlos changed the title ~~[WIP] build: add cmake and conan support~~ [WIP] build: add cmake support Mar 17, 2020

gocarlos changed the title ~~[WIP] build: add cmake support~~ build: add cmake support Mar 17, 2020

gocarlos force-pushed the feat--add-cmake-support branch 2 times, most recently from 1091321 to a7a1278 Compare March 18, 2020 07:44

lategoodbye requested changes Mar 18, 2020

View reviewed changes

mbus/mbus-protocol.h Outdated Show resolved Hide resolved

gocarlos force-pushed the feat--add-cmake-support branch from a7a1278 to 42caa33 Compare March 19, 2020 12:44

gocarlos requested a review from lategoodbye March 19, 2020 12:48

gocarlos force-pushed the feat--add-cmake-support branch from 561baba to ed61769 Compare March 20, 2020 14:41

gocarlos commented Mar 20, 2020

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

gocarlos mentioned this pull request Mar 24, 2020

feat: add libmbus conan-io/conan-center-index#1172

Merged

4 tasks

gocarlos and others added 2 commits March 31, 2020 21:38

test: update generate-xml script

b0f4130

build: apply suggestions from code review

fffdca0

Co-Authored-By: Anonymous Maarten <[email protected]>

gocarlos force-pushed the feat--add-cmake-support branch from 0d294a1 to fffdca0 Compare March 31, 2020 19:44

chore: apply suggestions from code review

505c25a

Co-Authored-By: Anonymous Maarten <[email protected]>

madebr reviewed Apr 1, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

gocarlos added 2 commits April 1, 2020 15:52

build: symplify clang tidy

b58aca4

chore: simplify cmake

989404d

madebr reviewed Apr 1, 2020

View reviewed changes

gocarlos and others added 3 commits April 1, 2020 22:38

chore: apply suggestions from code review

f3a62d5

Co-Authored-By: Anonymous Maarten <[email protected]>

build: add devel package

f31fbea

fix: unit tests

9f9c7a5

gocarlos added 2 commits April 14, 2020 11:22

docs: add better usage to generate xml script

ee3a69e

fix: debian package blank line in value of field Description

716fe03

chore: make path to tests optional

f569816

gocarlos requested a review from lategoodbye April 24, 2020 07:55

chore: run tests also with pull requests

ba18321

lategoodbye merged commit baf03e2 into rscada:master Apr 24, 2020

lategoodbye mentioned this pull request Jun 27, 2020

Running the tests on unmodified master gives printouts (diffs) #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add cmake support #151

build: add cmake support #151

gocarlos commented Mar 17, 2020

lategoodbye commented Mar 17, 2020 •

edited

Loading

gocarlos commented Mar 17, 2020 •

edited

Loading

gocarlos commented Mar 17, 2020

lategoodbye commented Mar 18, 2020

gocarlos commented Mar 18, 2020

Apollon77 commented Mar 18, 2020

lategoodbye commented Mar 21, 2020

gocarlos commented Mar 21, 2020 •

edited

Loading

lategoodbye commented Mar 21, 2020

lategoodbye commented Mar 21, 2020

gocarlos commented Mar 21, 2020

lategoodbye commented Mar 21, 2020

Apollon77 commented Mar 21, 2020

gocarlos commented Mar 22, 2020

Apollon77 commented Mar 22, 2020

Apollon77 commented Mar 22, 2020

gocarlos commented Mar 23, 2020

gocarlos commented Mar 31, 2020 •

edited

Loading

gocarlos commented Apr 1, 2020

madebr commented Apr 1, 2020

gocarlos commented Apr 14, 2020

gocarlos commented Apr 22, 2020

lategoodbye commented Apr 23, 2020

gocarlos commented Apr 24, 2020

lategoodbye commented Apr 24, 2020

lategoodbye commented Jun 27, 2020

gocarlos commented Jun 27, 2020

lategoodbye commented Jun 27, 2020

lategoodbye commented Jul 14, 2020

build: add cmake support #151

build: add cmake support #151

Conversation

gocarlos commented Mar 17, 2020

lategoodbye commented Mar 17, 2020 • edited Loading

gocarlos commented Mar 17, 2020 • edited Loading

gocarlos commented Mar 17, 2020

lategoodbye commented Mar 18, 2020

gocarlos commented Mar 18, 2020

Apollon77 commented Mar 18, 2020

lategoodbye commented Mar 21, 2020

gocarlos commented Mar 21, 2020 • edited Loading

lategoodbye commented Mar 21, 2020

lategoodbye commented Mar 21, 2020

gocarlos commented Mar 21, 2020

lategoodbye commented Mar 21, 2020

Apollon77 commented Mar 21, 2020

gocarlos commented Mar 22, 2020

Apollon77 commented Mar 22, 2020

Apollon77 commented Mar 22, 2020

gocarlos commented Mar 23, 2020

gocarlos commented Mar 31, 2020 • edited Loading

gocarlos commented Apr 1, 2020

madebr commented Apr 1, 2020

gocarlos commented Apr 14, 2020

gocarlos commented Apr 22, 2020

lategoodbye commented Apr 23, 2020

gocarlos commented Apr 24, 2020

lategoodbye commented Apr 24, 2020

lategoodbye commented Jun 27, 2020

gocarlos commented Jun 27, 2020

lategoodbye commented Jun 27, 2020

lategoodbye commented Jul 14, 2020

lategoodbye commented Mar 17, 2020 •

edited

Loading

gocarlos commented Mar 17, 2020 •

edited

Loading

gocarlos commented Mar 21, 2020 •

edited

Loading

gocarlos commented Mar 31, 2020 •

edited

Loading