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

Added initial CMakeLists file to compile with cmake #5

Closed
wants to merge 3 commits into from
Closed

Added initial CMakeLists file to compile with cmake #5

wants to merge 3 commits into from

Conversation

Talhada
Copy link
Contributor

@Talhada Talhada commented Feb 3, 2022

Descritption

Added cmake files to build the project.

Build

  • Make sure you have cmake installed (run sudo apt-get install cmake)
  • run mkdir build / cd build
  • run cmake .. to generate the makefiles (if linux system)
  • run make

@frno7
Copy link
Owner

frno7 commented Feb 3, 2022

This looks nice and tidy! As mentioned in issue #3, is it possible to do #include dependency generation too, similar to the following?

libpes/Makefile

Lines 44 to 50 in dea7ea0

# Header dependencies
HAVE_GCC_DEP:=$(shell touch .gcc-test.c && \
$(CC) -c -Wp,-MD,.gcc-test.d .gcc-test.c 2>/dev/null && \
echo 'yes'; rm -f .gcc-test.d .gcc-test.o .gcc-test.c)
ifeq ($(HAVE_GCC_DEP),yes)
BASIC_CFLAGS += -Wp,-MMD,$(BUILD_DIR)/$(@D)/$(@F).d -MT $(@D)/$(@F)
endif

The logic is essentially that CFLAGS is amended if and only if HAVE_GCC_DEP works with the chosen compiler. Perhaps CMake has other methods for it, but I wouldn’t want it to download any packages; it must be built-in, in this case.

Also, is it possible to have a Make target test that actually runs the tests, like this (with some means to control the verbosity):

libpes/test/Makefile

Lines 10 to 12 in dea7ea0

.PHONY: test
test: $(BUILD_DIR)/run-tests
$(QUIET_TEST)$(BUILD_DIR)/run-tests $(if $(V:@=),-v)

Some files have missing newlines.

@Talhada
Copy link
Contributor Author

Talhada commented Feb 3, 2022

Hi, thank you for the quick reply!

Dependency Generation

Not sure if I understand the question, I'm rusty on Makefile syntax :)
If you mean passing different flags for different compilers, that is possible with standard cmake, for example:

if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
  # using Clang
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
  # using GCC
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "Intel")
  # using Intel C++
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
  # using Visual Studio C++
endif()

You can use this to add preprocessor definitions to be used in your header files:

add_definitions(-DFOO -DBAR ...)

and then on the header file one can use:

#ifdef FOO
... 
#endif
#ifdef BAR
... 
#endif

For the rest, the build process is done automatically, even if you change the CMake or the source files, you only have to do Make, CMake will re-run itself when required (example: adding a new source file). By looking at the project I can't see any dependency on headers.

If you mean automatic header creation by the use of ./configure then yes, it is also possible with CMake by using the function configure_file()

Custom targets

Yes, sure thing. It is possible. CMake has also built in functions to add tests to the project.

Missing newlines

Maybe Github has an action to format files on commit to avoid those issues. I'll check.

Let me know your thoughts,
Thank you

@frno7
Copy link
Owner

frno7 commented Feb 4, 2022

It’s much better to check whether a particular compiler feature works, than naming particular compilers, because GCC and Clang, for example, have many features in common, and different features in different versions, and may obtain new features in the future. Likewise with MSVC. What I believe CMake should do is to run the shell script (or similar)

libpes/Makefile

Lines 45 to 47 in dea7ea0

HAVE_GCC_DEP:=$(shell touch .gcc-test.c && \
$(CC) -c -Wp,-MD,.gcc-test.d .gcc-test.c 2>/dev/null && \
echo 'yes'; rm -f .gcc-test.d .gcc-test.o .gcc-test.c)

and if the answer is yes, it would update CFLAGS like

BASIC_CFLAGS += -Wp,-MMD,$(BUILD_DIR)/$(@D)/$(@F).d -MT $(@D)/$(@F)

These header dependencies mean that whenever one edits a header file, all the C files that depend on (#include) it will be recompiled automatically. GCC can do it, but I don’t know about Clang or MSVC. If Clang doesn’t, perhaps it will in the future.

Git Hub indicates red icons for c720812 on the last line of certain files, for example the file test/CMakeLists.txt. The icon has a text that says no newline at end of file, so I assume if your editor didn’t add the last newline, it should be simple to add it manually? Most editors that I know of automatically add newlines, but maybe some don’t.

@Talhada
Copy link
Contributor Author

Talhada commented Feb 4, 2022

CMake build system

With cmake builds you do not need to care about header dependencies. The build system will take care of that for you. Now what build system you use, depends vastly on the platform: On unix it may be makefiles or ninja, but on windows for example one can use nmake, ninja or even generate visual studio projects. That can all be done with cmake. The only important thing to consider is to always add the header files as part of the project build rules on CMakeLists.txt. Everything is taken care automatically after that.

However it is always possible to add additional flags to the compiler if required, but usually I don't see that very often.
Usually you run cmake for Debug/Release builds to turn symbols and optimizations On/Off.

Screenshot from 2022-02-04 22-11-49

When you run CMake for the first time it will automatically detect the compiler and the features available on the system

Newlines

Well I am using Visual Studio 2022, should have a setting for that otherwise I'll fix it manually.

@frno7
Copy link
Owner

frno7 commented Feb 5, 2022

Great, @Talhada! :-) Indeed, CMake has the dependencies I was looking for:

% head src/CMakeFiles/libpes.dir/sax.c.o.d
src/CMakeFiles/libpes.dir/sax.c.o: \
 .../src/sax.c /usr/include/stdc-predef.h \
 /usr/lib/gcc/powerpc64le-unknown-linux-gnu/11.2.0/include/stdbool.h \
 /usr/include/stdlib.h /usr/include/bits/libc-header-start.h \

How would you propose the tests be made and run with CMake? The Makefiles offered two variants, a somewhat quiet

% make test
     LINK     build/ppc64le/run-tests
     TEST     test

and another with the verbosity flag V=1

% make test V=1
cc -Wall -O2 -g -Iinclude -Wp,-MMD,build/ppc64le/build/ppc64le/run-tests.d -MT \
	build/ppc64le/run-tests -o build/ppc64le/run-tests \
	build/ppc64le/test/run-tests.o build/ppc64le/test/sax-tests.o \
	build/ppc64le/test/svg-transcoder-tests.o \
	build/ppc64le/libpes.a -lm
build/ppc64le/run-tests -v
Testing SAX
  test_sax_parser... OK
  test_sax_strcmp... OK
Testing SVG transcoder
  test_svg_transcoder... OK

the latter which runs run-tests -v. This test target seems to be all there’s left to completely replace the Makefiles with CMake.

@Talhada
Copy link
Contributor Author

Talhada commented Feb 5, 2022

Adding tests is pretty much straightforward:

  1. Enable testing on the main CMake File:
enable_testing()
  1. On the cmake file that contains the rules to compile the test executable:
add_test(NAME run-tests COMMAND run-tests)

After this two instructions the command "make test" will run the test executable as follows:
Screenshot from 2022-02-05 18-27-12

However I still need to figure how to make the -v flag work.

@frno7
Copy link
Owner

frno7 commented Feb 5, 2022

Nice! Would the printing from testing with -v mangle and confuse the output of CMake itself? If so, I think we could skip it, although it’s always reassuring to actually see what is being tested.

@Talhada
Copy link
Contributor Author

Talhada commented Feb 7, 2022

So, the only thing that comes to my mind (keeping the test code as is) is the following snippet:

# Run tests silently ('make test' or 'ctest')
add_test(NAME run-tests COMMAND run-tests)

# Run tests with verbose information ('make test-v' or 'cmake --build . --target test_v')
add_custom_target(test-v run-tests
    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/run-tests -v
    VERBATIM
)

The first line is the standard way to add a test to a CMake Build system. Nothing new.
Then I used the add_custom_target function to make available a new target that runs the test program with the verbose flag on.

with this you can run make test and obtain the same result as before:
Screenshot from 2022-02-07 18-14-13

and 'make test-v' to get the standard verbose output:
Screenshot from 2022-02-07 18-15-26

@frno7
Copy link
Owner

frno7 commented Feb 7, 2022

Thanks, it looks great! This seems ready to merge to me, unless you want to adjust anything?

The last lines in commit e9096ff has red warning icons again displaying no newline at end of file, but I can fix them.

@Talhada
Copy link
Contributor Author

Talhada commented Feb 7, 2022

Great, I've also added an action to build and test the entire project on each change for the master branch.
It shows if the project is currently building and passing all the tests

It looks like:
Screenshot from 2022-02-07 18-34-55

@frno7
Copy link
Owner

frno7 commented Feb 8, 2022

@Talhada, thank you for your contribution! I’ve merged your changes in commits 0a62a94, 2be1435, and fb702e6, including the build action.

@frno7 frno7 closed this Feb 8, 2022
@Talhada
Copy link
Contributor Author

Talhada commented Feb 8, 2022

Thank tou for your time doing the merges and explain all the requirements for me. Glad to contribute.

@frno7 frno7 mentioned this pull request Feb 8, 2022
@Talhada Talhada deleted the cmake-integration branch February 8, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants