CI improvements #51

janheinrichmerker · 2025-03-11T01:19:55Z

Run Ubuntu 20.04 as Docker image (Improve Build Matrix as discussed in #34 #37, Improve OS Support [C CI] #34)
Add Ubuntu 18.04 support ~~(impossible due to: /__e/node20/bin/node: /lib/x86_64-linux-gnu/libc.so.6: version GLIBC_2.28' not found (required by /__e/node20/bin/node)` actions/checkout#1590)~~ turns out, it is not impossible when building inside Docker: job/38747725331
Test CLI and Debian build in Build matrix (Improve Build Matrix as discussed in #34 #37, Improve OS Support [C CI] #34)
Add Ubuntu 16.04 support?
Fix Windows builds
Add Docker integration tests (Add Docker integration tests #49)

codecov · 2025-03-11T02:50:30Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.27%. Comparing base (7e33d4a) to head (9649f0c).
Report is 56 commits behind head on master.

Files with missing lines	Patch %	Lines
...y/src/main/kotlin/io/tira/tirex/tracker/Tracker.kt	0.00%	0 Missing and 1 partial ⚠️
python/tirex_tracker/__init__.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #51      +/-   ##
============================================
- Coverage     78.36%   78.27%   -0.09%     
  Complexity       20       20              
============================================
  Files             3        3              
  Lines          1151     1151              
  Branches         53       54       +1     
============================================
- Hits            902      901       -1     
  Misses          208      208              
- Partials         41       42       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

c/CMakeLists.txt

TheMrSheldon

Thank you very much for the changes so far! Though this is still a draft PR, I have reviewed some parts of it to give early feedback on some concerns I have.

Note also that the switch to C++20 is quite drastic and I believe that, the code should at least not run on MSVC (even with the configuration errors ammended that I pointed out in my review). Some language features that are used here were introduced in C++20. E.g., designated initializers which are used in various places). I know that g++ and clang++ are quite leanient with these but some compilers (AFAIK especially MSVC) are not and we can't claim C++17 compatibility yet.

for designated initializers I suggest that we simply comment out the variable names to keep readability intact.

TheMrSheldon · 2025-03-16T13:49:11Z

c/CMakeLists.txt

+	FetchContent_Declare(doxawesome GIT_REPOSITORY https://github.com/jothepro/doxygen-awesome-css GIT_TAG v2.3.4)
+	FetchContent_MakeAvailable(doxawesome)


${doxawesome_SOURCE_DIR} above is populated with FetchContent. As such, FetchContent must precede it.

TheMrSheldon · 2025-03-16T13:56:45Z

c/cli/CMakeLists.txt

+	set(CPACK_GENERATOR "DEB" CACHE STRING "" FORCE)
+	set(CPACK_PACKAGE_NAME "tirex-tracker" CACHE STRING "" FORCE)
+	set(CPACK_SET_DESTDIR TRUE CACHE PATH "" FORCE)
+	set(CPACK_DEBIAN_PACKAGE_MAINTAINER "Tim Hagen" CACHE STRING "" FORCE)
+	set(CPACK_PACKAGE_VERSION "${TIREX_TRACKER_VERSION}" CACHE STRING "" FORCE)
+	set(CPACK_DEBIAN_PACKAGE_DESCRIPTION "Automatic resource and metadata tracking for information retrieval experiments." CACHE STRING "" FORCE)
+	set(CPACK_RESOURCE_FILE_README "${CMAKE_CURRENT_SOURCE_DIR}/../../README.md" CACHE PATH "" FORCE)
+	set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/../../LICENSE" CACHE PATH "" FORCE)
+	set(CPACK_DEBIAN_PACKAGE_DEPENDS "" CACHE STRING "" FORCE)
+	set(CPACK_PACKAGE_VENDOR "" CACHE STRING "" FORCE)


Should be checked but I don't think they need to be forced and caching them should also not be necessary.

TheMrSheldon · 2025-03-16T13:58:31Z

c/cli/CMakeLists.txt

+if(NOT TIREX_TRACKER_ONLY_DOCS AND (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) AND TIREX_TRACKER_BUILD_DEB)
+	# Include helper module.
+	include(CPack)
+	include(GNUInstallDirs)  # FIXME: Is this needed?


Where is this used?

TheMrSheldon · 2025-03-16T14:00:57Z

c/cli/CMakeLists.txt

+# The Debian Packaging build target is only available if this is the root project
+if(NOT TIREX_TRACKER_ONLY_DOCS AND (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) AND TIREX_TRACKER_BUILD_DEB)
+	# Include helper module.
+	include(CPack)


The include must come after the configuration. See here:

Before including this CPack module in your CMakeLists.txt file, there are a variety of variables that can be set to customize the resulting installers.

TheMrSheldon · 2025-03-16T14:01:32Z

c/cli/CMakeLists.txt

+##################
+# The Debian Packaging build target is only available if this is the root project
+if(NOT TIREX_TRACKER_ONLY_DOCS AND (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) AND TIREX_TRACKER_BUILD_DEB)
+	# Include helper module.


The comment seems redundant to me. It does not really explain anything that is not directly obvious by the line below.

TheMrSheldon · 2025-03-16T15:00:23Z

c/src/measureapi.cpp

@@ -37,8 +36,8 @@ struct tirexMeasureHandle_st final {
 		monitorthread.join();

 		// Stop measuring
-		for (auto& provider : providers | std::views::reverse)
-			provider->stop();
+		for (std::size_t i = providers.size() - 1; providers.size() > i; --i)


I would personally prefer a reverse iterator directly instead of relying on a random access container.

TheMrSheldon · 2025-03-16T15:21:25Z

c/src/measureresult.cpp

+				[key, &result](auto&& arg) {
+					using T = std::decay_t<decltype(arg)>;
+					if constexpr (std::is_same_v<T, std::string>)
+						result.emplace_back(key, std::move(arg));
+					else if constexpr (std::is_same_v<T, tirex::TimeSeries<unsigned>>)
+						result.emplace_back(key, toYAML(arg));


I don't see the advantage over overloading. Especially since the type is then again checked within the function. This doubles the number of conditions per call and looks entirely odd, wheras overloading is a standard design pattern. If overloading is not preferred, the visit should be made explicit via get_if.

TheMrSheldon · 2025-03-16T15:22:32Z

c/src/measureresult.cpp

-struct overloaded : Ts... {
-	using Ts::operator()...;
-};
+static std::string toYAML(const tirex::TimeSeries<unsigned>& timeseries) {


Why does this only work for tirex::TimeSeries<unsigned>?

TheMrSheldon · 2025-03-16T15:22:57Z

c/src/measureresult.cpp

+	std::stringstream timestampsStream;
+	timestampsStream << "[";
+	bool firstTImestamp = true;
+	for (auto& elem : timestamps) {
+		if (!firstTImestamp)
+			timestampsStream << ",";
+		timestampsStream << "\"";
+		timestampsStream << elem.count();
+		timestampsStream << "\"";
+		firstTImestamp = false;
+	}
+	timestampsStream << "]";

-template <class... Ts>
-overloaded(Ts...) -> overloaded<Ts...>;
+	std::stringstream valuesStream;
+	valuesStream << "[";
+	bool firstValue = true;
+	for (auto& elem : values) {
+		if (!firstValue)
+			valuesStream << ",";
+		valuesStream << "\"";
+		valuesStream << elem;
+		valuesStream << "\"";
+		firstValue = false;
+	}
+	valuesStream << "]";


Should be split into its own function that is called once for timestamps and once for values.

TheMrSheldon · 2025-03-16T15:28:04Z

c/src/measure/stats/systemstats.cpp

+		if (unified) {
+			if (!first)
+				cachesStream << ",";
+			cachesStream << _fmt::format("\"l{}\": {}", cacheIdx, unified);
+			first = false;
+		}
+		if (instruct) {
+			if (!first)
+				cachesStream << ",";
+			cachesStream << _fmt::format("\"l{}i\": {}", cacheIdx, instruct);
+			first = false;
+		}
+		if (data) {
+			if (!first)
+				cachesStream << ",";
+			cachesStream << _fmt::format("\"l{}d\": {}", cacheIdx, data);
+			first = false;
+		}


To remove all the conditions, I would preferr a delim_seq that is initially "" and the set to ",". Maybe something like this:

Suggested change

if (unified) {

if (!first)

cachesStream << ",";

cachesStream << _fmt::format("\"l{}\": {}", cacheIdx, unified);

first = false;

}

if (instruct) {

if (!first)

cachesStream << ",";

cachesStream << _fmt::format("\"l{}i\": {}", cacheIdx, instruct);

first = false;

}

if (data) {

if (!first)

cachesStream << ",";

cachesStream << _fmt::format("\"l{}d\": {}", cacheIdx, data);

first = false;

}

if (unified) {

cachesStream << delim << _fmt::format("\"l{}\": {}", cacheIdx, unified);

delim = ",";

}

if (instruct) {

cachesStream << delim << _fmt::format("\"l{}i\": {}", cacheIdx, instruct);

delim = ","

}

if (data) {

cachesStream << delim << _fmt::format("\"l{}d\": {}", cacheIdx, data);

delim = ",";

}

_fmt::format may also not be necessary anymore since we use stringstream here

Important consideration: Note that ir_metadata always gives string values where the unit is appended seoarated by a space (see for example for RAM here). Our previous extension of ir_metadata respected this design decision and adhered to it. It would be removed with the changes proposed here.

TheMrSheldon · 2025-03-16T15:50:30Z

c/src/measure/stats/systemstats.cpp

+bool isBigEndian() {
+	// Store 0x0001 in memory.
+	uint16_t word = 1;
+	// Get a pointer to the first byte of the word.
+	uint8_t* firstByte = (uint8_t*)&word;
+	// Check if the first byte is zero.
+	return !(*firstByte);
+}


I will continue the discussion from 25d4ea6#r153677577 here so that it is more visible. The proper way in C++ would be bit_cast(https://en.cppreference.com/w/cpp/numeric/bit_cast) though this is a C++20 feature. reinterpret_cast is typically used here though it would not be standard conformant though as far as casts are concered, I would say it is the closest one that is semantically correct without using bit_cast. union is also not standard compliant as only one member of a union may be active at any one time and the rest is UB. So the cleanest solution for a runtime check outside of C++20 would be writing an own bit_cast using memcpy. But endianness should be known at compile time (since constants are also written to the binary) so that the endianness macros of typical compilers can be used (in the end, std::endian only unifies this interface). Such an implementation is given here.

janheinrichmerker added 6 commits March 10, 2025 12:14

Re-enable Debian package matrix build

f230ed8

Use latest runner for non-version-specific CI jobs

48e4116

Use Docker runner for Ubuntu 20.04

6df9d4e

Add workaround

77aec89

Update clang version

6c9d8d3

Add workaround

6dca5b8

janheinrichmerker added the enhancement New feature or request label Mar 11, 2025

janheinrichmerker self-assigned this Mar 11, 2025

janheinrichmerker marked this pull request as draft March 11, 2025 01:20

janheinrichmerker added 9 commits March 11, 2025 02:28

Require C++ standard explicitly

7ca0fa5

Fix workaround

6a1a046

Simplify CI, add Ubuntu 18.04

6a635f6

Downgrade action for compatibility with Ubuntu 18.04

09cbdbc

Downgrade actions for compatibility with Ubuntu 18.04

a739b05

Fix acition version

19d7cda

Fix acition version

5a63901

Remove Ubuntu 18.04 support

32dfcab

Downgrade clang-format

a87a382

janheinrichmerker linked an issue Mar 11, 2025 that may be closed by this pull request

Improve Build Matrix as discussed in #34 #37

Open

Fix Windows library path

5d4f0c3

janheinrichmerker added 2 commits March 11, 2025 03:58

Fix path

965bc76

Fix build exclusions

c30bdb7

TheMrSheldon reviewed Mar 11, 2025

View reviewed changes

c/CMakeLists.txt Outdated Show resolved Hide resolved

janheinrichmerker added 6 commits March 11, 2025 19:47

Re-enable Ubuntu 18.04

da439bb

Fix comparison

886ebaa

Backport checkout on Ubuntu 18.04

f12e502

Fix legacy check out

afb1a7c

Backport checkout on Ubuntu 18.04

6df44e9

Build 22.04 and 18.04 in Docker

99f0f8a

janheinrichmerker added 28 commits March 13, 2025 06:01

Fix CI

e0e3965

Fix code format

17f6c57

Add Ubuntu 16.04 CI

cc056ac

Fix Debian package CI

834c5b3

Reformat .clang-format

e7a2cd6

Improve C format, enforce C++ standard

2ba0053

Downgrade C++ standard to 17

7ba9cc3

Fix C format

ca21dff

Improve clang-format command

b31d35a

Backport endianness

25d4ea6

Improve CMake build

2453c46

Remove unused definitions

facd57e

Fix CMake build

56a98db

Fix CMake build

df945f0

Simplify sleep

f06fe34

Link utilities

53be9b0

Simplify sleep

ce963dc

Fix CMake build

2c1c2a6

Fix C format check

125ad71

Disable Ubuntu 16.04 build

204c599

Fix CMake build

9519adf

Disable Windows build temporarily

61e9c7b

Fix CMake build

3918684

Add commented-out matrix builds again

b5ce863

Fix CMake build

2e66af5

Fix CMake build

56c3012

Fix build

e037005

Fix build

e00a8b8

TheMrSheldon reviewed Mar 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI improvements #51

CI improvements #51

janheinrichmerker commented Mar 11, 2025 •

edited

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading

TheMrSheldon left a comment

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

TheMrSheldon Mar 16, 2025

		FetchContent_Declare(doxawesome GIT_REPOSITORY https://github.com/jothepro/doxygen-awesome-css GIT_TAG v2.3.4)
		FetchContent_MakeAvailable(doxawesome)

CI improvements #51

Are you sure you want to change the base?

CI improvements #51

Conversation

janheinrichmerker commented Mar 11, 2025 • edited Loading

codecov bot commented Mar 11, 2025 • edited Loading

Codecov Report

TheMrSheldon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janheinrichmerker commented Mar 11, 2025 •

edited

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading