Things to Improve - Githubissues

`README.md`

While building I suggest cmake --build . --parallel instead of cmake --build . so it uses all your cores for building.
On fedora ostree-devel needs to be installed first (if you do not build it via CMake).

OSTree as Build in Dependency

The best CMake project is a project you clone and it just compiles and grabs all dependencies for you. Besides that you have a known good fixed version of OSTree you are linking against. That's why using system dependencies is not a good idea generally for everything that is fast enough to build.

The following things are required: sudo dnf install gpgme-devel e2fsprogs-devel fuse-devel

Then here is a git patch for it:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cae0346..856940d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -32,6 +32,34 @@ if(NOT clip_POPULATED)
   add_subdirectory(${clip_SOURCE_DIR} ${clip_BINARY_DIR})
 endif()

+# OSTree______________________________________________________
+include(ExternalProject)
+
+ExternalProject_Add(ostree_build GIT_REPOSITORY https://github.com/ostreedev/ostree.git
+                                  GIT_TAG v2024.5
+                                  PATCH_COMMAND cd "<SOURCE_DIR>" && env NOCONFIGURE=1 ./autogen.sh
+                                  CONFIGURE_COMMAND cd "<SOURCE_DIR>" && ./configure --enable-man=off
+                                  BUILD_COMMAND cd "<SOURCE_DIR>" && make -j 10
+                                  CONFIGURE_HANDLED_BY_BUILD ON
+                                  INSTALL_COMMAND ""
+                                  BUILD_IN_SOURCE OFF
+                                  UPDATE_DISCONNECTED ON
+                                  COMMENT "Building OSTree"
+                                  BUILD_BYPRODUCTS "<SOURCE_DIR>/.libs/libostree-1.so")
+
+add_library(libostree SHARED IMPORTED)
+set(SHUMATE_LIBRARIES "libostree")
+add_dependencies(libostree ostree_build)
+
+ExternalProject_Get_Property(ostree_build SOURCE_DIR)
+set(OSTREE_LIB "${SOURCE_DIR}/.libs/libostree-1.so")
+set_target_properties(libostree PROPERTIES IMPORTED_LOCATION ${OSTREE_LIB})
+
+set(OSTREE_INCLUDE_DIRS "${SOURCE_DIR}/src/libostree")
+# Fix for ExternalProject_Add only cloning during build not configure
+file(MAKE_DIRECTORY ${OSTREE_INCLUDE_DIRS})
+target_include_directories(libostree INTERFACE ${OSTREE_INCLUDE_DIRS})
+
 # Project _____________________________________________________
 set(PROJECT_NAME "ostree-tui")
 set(PROJECT_DESCRIPTION "Terminal User Interface for ostree.")
diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt
index 99db0a4..e7ba0da 100644
--- a/src/util/CMakeLists.txt
+++ b/src/util/CMakeLists.txt
@@ -2,7 +2,6 @@ cmake_minimum_required(VERSION 3.27)

 find_package(PkgConfig REQUIRED)
 pkg_check_modules(glib-2.0 REQUIRED IMPORTED_TARGET glib-2.0)
-pkg_check_modules(ostree-1 REQUIRED IMPORTED_TARGET ostree-1)

 file(GLOB_RECURSE util_sources *.cpp *.h)

@@ -11,7 +10,6 @@ add_library(util STATIC ${util_sources})
 target_include_directories(util
     PUBLIC
     ${glib-2.0_INCLUDE_DIRS}
-    ${ostree-1_INCLUDE_DIRS}
 )

 target_link_libraries(util
@@ -20,7 +18,7 @@ target_link_libraries(util
   PUBLIC /usr/lib64/libgio-2.0.so
   PUBLIC /usr/lib64/libgobject-2.0.so

-  PRIVATE ostree-1
+  PUBLIC libostree
   PRIVATE clip
 )

diff --git a/src/util/cpplibostree.h b/src/util/cpplibostree.h
index 4385d8e..fa78ae6 100644
--- a/src/util/cpplibostree.h
+++ b/src/util/cpplibostree.h
@@ -24,7 +24,7 @@
 #include <fcntl.h>
 // external
 #include <glib-2.0/glib.h>
-#include <ostree-1/ostree.h>
+#include <ostree.h>

 struct Signature {
     std::string pubkey_algorithm;

PKG Config For GLib

Providing absolute paths for libs is never a good idea. Therefore use a more flexible way of detecting external dependencies. Note for glib it is not really feasible to recompile it during setup and you can expect it being installed since it would take a loooong time and most likely introduce incompatibilities with the system version of glib (as I have experienced not just once :D).

Here is a patch for it:

diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt
index e7ba0da..3af0832 100644
--- a/src/util/CMakeLists.txt
+++ b/src/util/CMakeLists.txt
@@ -1,7 +1,11 @@
 cmake_minimum_required(VERSION 3.27)

 find_package(PkgConfig REQUIRED)
+
+set(ENV{PKG_CONFIG_PATH} "/usr/lib/pkgconfig")
 pkg_check_modules(glib-2.0 REQUIRED IMPORTED_TARGET glib-2.0)
+pkg_check_modules(gio-2.0 REQUIRED IMPORTED_TARGET gio-2.0)
+pkg_check_modules(gobject-2.0 REQUIRED IMPORTED_TARGET gobject-2.0)

 file(GLOB_RECURSE util_sources *.cpp *.h)

@@ -14,12 +18,10 @@ target_include_directories(util

 target_link_libraries(util
   PUBLIC PkgConfig::glib-2.0
-  # PkgConfig::glib-2.0 not including gio & gobject -> manually added
-  PUBLIC /usr/lib64/libgio-2.0.so
-  PUBLIC /usr/lib64/libgobject-2.0.so
-
-  PUBLIC libostree
-  PRIVATE clip
+         libostree
+  PRIVATE PkgConfig::gio-2.0
+          PkgConfig::gobject-2.0
+          clip 
 )

 add_library(ostui::util ALIAS util)

Note: As a rule of thumb, you link against something with PUBLIC in case it's included in a header that can be used by an other component outside your library. Everything else you link PRIVATE.

Clang-Format

Use clang format! You have the config, so please use it ;) If you are using VSCode, install the clangd extension. Nowadays it's a must have for modern C++ development.

Btw. It also handles automatic header grouping ;)

Header Files

Headers in C++ are called .hpp, not .h. I know we in the past also did not do that. But for all new projects we are actively enforcing it.

C++ Time Types

If you create a C++ wrapper, use C++ types. Use std::chrono::time_point instead of this: https://github.com/AP-Sensing/ostree-tui/blob/300a02866ee6619ebfbadead4b4779ab2ca339af/src/util/cpplibostree.h#L32C1-L32C26

If you need help with parsing them, take a look at the date.h lib which is just awesome and even slowly but surely gets integrated into C++. https://github.com/HowardHinnant/date/blob/master/include/date/date.h

It has really great CMake fetch_content support. Just take a look at our internal projects ;)

Uninitialised Types

https://github.com/AP-Sensing/ostree-tui/blob/300a02866ee6619ebfbadead4b4779ab2ca339af/src/util/cpplibostree.cpp#L154-L157

This is a perfect case for accessing uninitialised memory in case g_variant_get_child would ever not override the pointer. Always init it with const char *fingerprint{nullptr};

And since you are working with raw pointers why don't use assert(fingerprint) (#include <cassert>) after each g_variant_get_child?

If you want, you can also take a look at gsl which provides more flexible asserts that are also existent during release builds. Modern C++ is all about memory safety!

NULL

Don't use NULL, use nullptr in C++. nullptr is a pointer type which is far better in that case.

Clang-Tidy

Again, use clangd (see above). It really gives and teaches you how to write good C++ code in a lot of ways. (c-style casts, else after return, ...)

`==` Check

https://github.com/AP-Sensing/ostree-tui/blob/300a02866ee6619ebfbadead4b4779ab2ca339af/src/main.cpp#L11

Again, potential for a out of bounds access. If I manage to invoke your prog with -1 as argc, I can do everything.

Always check ranges when ever you can and avoid ==. In this case use if (argc <= 0) {.

CMake File Globs

https://github.com/AP-Sensing/ostree-tui/blob/main/src/core/CMakeLists.txt

file(GLOB_RECURSE ostree_sources *.cpp *.h)

Evil! Since this can lead to a loooot of bugs! Always specify all files explicitly in CMake. Example: https://github.com/COM8/aps-ui/blob/main/src/ui/widgets/CMakeLists.txt

Using namespaces In Headers

Something like using namespace ftxui; inside a header file is evil since it pollutes the header space of everyone who includes it. A local using namespace ftxui; in a closed scope OK. Don't like it (I'm always preferring explicit).

Define Namespaces

Define namespaces! Don't have everything inside the global namespace. Namespaces usually follow the directory structure. Example: Files inside src/core should be inside the otui::core namespace (if you use otui as an abbreviation for ostree-tui).

Lambdas

Yes, lambdas are cool I know, but a private function is also cool sometimes if it just appears inside a .cpp file or an anonymous namespace ;). Ref: https://releases.llvm.org/16.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc/use-anonymous-namespace.html

Closing Remarks

I really like the way you structure your code and who you document it. This is really something you can build up on top of. It really looks like your first real "large" C++ project, but that's good. You learn and I'm trying to push you a bit into the right directions here and there :)

Keep up the good work.

AP-Sensing / ostree-tui

Things to Improve #7

`README.md`

OSTree as Build in Dependency

PKG Config For GLib

Clang-Format

Header Files

C++ Time Types

Uninitialised Types

NULL

Clang-Tidy

`==` Check

CMake File Globs

Using namespaces In Headers

Define Namespaces

Lambdas

Closing Remarks

AP-Sensing / ostree-tui

Things to Improve #7

README.md

OSTree as Build in Dependency

PKG Config For GLib

Clang-Format

Header Files

C++ Time Types

Uninitialised Types

NULL

Clang-Tidy

== Check

CMake File Globs

Using namespaces In Headers

Define Namespaces

Lambdas

Closing Remarks

`README.md`

`==` Check