Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for building sngrep using CMake added #462

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

axelsommerfeldt
Copy link
Contributor

The file CMakeLists.txt was added to enable building sngrep with CMake. (This file was derived from configure.ac.)
Furthermore a paragraph about building with CMake was added to the README.

…e a pkg-config file

Furthermore CPACK_PACKAGE_NAME and CPACK_PACKAGE_VERSION are set explicitly since this is needed for older CMake versions.

(Tested on Debian Stretch with CMake 3.7.2)
@Kaian
Copy link
Member

Kaian commented Nov 13, 2023

Hi!

Thanks for the contribution.

There is an stale branch where I started to code sngrep using glib2 that used cmake for building.
Check https://github.com/irontec/sngrep/blob/glib-2/CMakeLists.txt in case it could provide any help or inspiration.

I'll try to check this as soon as I have some spare time

Thanks!

@axelsommerfeldt
Copy link
Contributor Author

There is an stale branch where I started to code sngrep using glib2 that used cmake for building.

Oh, I didn't know this, otherwise I would have used this for a start. Instead I have used configure.ac as template and even tried to keep the instruction order as close as possible so keeping them in sync in the future should not be too hard. (Furthermore I read the project version from configure.ac so it only needs to be updated in one place.)

Check https://github.com/irontec/sngrep/blob/glib-2/CMakeLists.txt in case it could provide any help or inspiration.

  • -Werror -Wall -Wextra -pedantic is always a good idea, should I add this, too?
  • Yours adds _XOPEN_SOURCE_EXTENDED as definition, is this used/needed by sngrep? (My CMakeLists.txt defines _GNU_SOURCE only.)
  • Yours have a minimum version 3.0 for CMake, mine requires at least version 3.7. Is version 3.7 ok as requirement? (Otherwise I have to change a lot of stuff since I use "modern CMake" instructions which are not really usable until version 3.5.)

@axelsommerfeldt
Copy link
Contributor Author

...and using configure_file() seems to be a better approach than writing config.h inside CMakeLists.txt, I will inherit this from your CMakeLists.txt, too.

@Kaian
Copy link
Member

Kaian commented Nov 13, 2023

  • -Werror -Wall -Wextra -pedantic is always a good idea, should I add this, too?

Looks good to me, but is the code good enough to compile with those flags? :-D

  • Yours adds _XOPEN_SOURCE_EXTENDED as definition, is this used/needed by sngrep? (My CMakeLists.txt defines _GNU_SOURCE only.)

Most probably I added those defines because glib-2 require them, but maybe it is not required for sngrep 1.x

  • Yours have a minimum version 3.0 for CMake, mine requires at least version 3.7. Is version 3.7 ok as requirement? (Otherwise I have to change a lot of stuff since I use "modern CMake" instructions which are not really usable until version 3.5.)

No, use what you prefer. That CMakefile is from a long time ago and it's my first CMakefile, so most probably should not be used as an example. I just added the command in case you can take some ideas, but I'm far from an expert in this topic.

Thanks!!

@axelsommerfeldt axelsommerfeldt marked this pull request as draft November 13, 2023 15:49
- Compiler options '-Wall -pedantic' added
- Definition of _XOPEN_SOURCE_EXTENDED added
- Template configuration header file src/config.h.cmake added
- README updated

See also: irontec#462
@axelsommerfeldt
Copy link
Contributor Author

axelsommerfeldt commented Nov 13, 2023

  • -Werror -Wall -Wextra -pedantic is always a good idea, should I add this, too?

Looks good to me, but is the code good enough to compile with those flags? :-D

At least -Wall -pedantic leads to only a few warnings, therefore I have added them. Is this ok for you?

  • Yours adds _XOPEN_SOURCE_EXTENDED as definition, is this used/needed by sngrep? (My CMakeLists.txt defines _GNU_SOURCE only.)

Most probably I added those defines because glib-2 require them, but maybe it is not required for sngrep 1.x

I have added it anyway since it should not make any harm.

but I'm far from an expert in this topic.

I once have read "Has written a book about it or has given a talk at a conference about it" as definition for "expert". Using this definition I'm not an expert either, I'm only a user for a few years, nothing more.

And some stuff is simple a matter of taste. For foreign projects I usually tend to avoid using configure_file() since an additional file is required for this and some maintainers don't like this, they are willing to accept a single CMakeLists.txt but nothing more. Since this does not apply to you I have changed it according to your CMakeLists.txt in the glib-2 branch.

@axelsommerfeldt axelsommerfeldt marked this pull request as ready for review November 13, 2023 18:28
@Kaian
Copy link
Member

Kaian commented Nov 29, 2023

Looks awesome to me.

Thanks a lot for your effort and hard work on this!

@Kaian Kaian merged commit a4a2357 into irontec:master Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants