Move headers Part 3 #4941

kodebach · 2023-05-22T20:57:22Z

What happens in this PR?

With #4940 the pure header updates for should be concluded. However, the decisions listed in #4920 also include some additional changes like new libraries.

This PR includes many CMake changes and finally splits src/libs/elektra properly into src/libs/core and src/libs/kdb.

Among the CMake changes, is a change in how BUILD_FULL and BUILD_STATIC work. Instead of collecting all the source and header files and compiling them again, we just collect all the CMake targets and use the $<TARGET_OBJECTS:...> syntax to add everything as dependencies.

The PR also introduces two new libraries src/libs/base and src/libs/pluginload. These are not actual libraries that can be installed. Instead, as described in the decisions, they are OBJECT libraries in CMake, meaning the .c files are compiled into .o files but nothing more. We do this, so we can extract base functionality (like elektraMalloc) and plugin loading from elektra-core and elektra-kdb, without introducing additional dependencies.

The PR also introduces a new src/libs/type, which is just a bit of src/libs/ease extracted into a separate library.

Finally, this PR has a change that was hinted at in #4940 (comment). It turns quite a few functions which are just small wrappers around stdlib functions (elektraMalloc, elektraFree, etc.) into static inline functions in internal headers. The way this is done is a bit quirky, but it works and allowed the compiler to do more in-depth analysis (see 679085b).

Hints for reviewers

The PR is a bit bigger, because there are many (similar) changes to CMake files, but otherwise it should be like any other PR in terms of reviews. All commits where done manually.

Most of the CMake changes are just removing add_headers and other stuff that isn't actually needed. If you don't understand something, just ask.

Basics

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which contains _(my name)_)
Please always add them to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
- fix the CI itself (or rebase if already fixed)
The PR is rebased with current master.

Checklist

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation (see Documentation Guidelines)
I fixed all affected decisions (see Decision Process)
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in reuse syntax

Review

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if everything is done and no further pushes are planned by you.

kodebach · 2023-05-22T21:15:20Z

As a preview: After this PR, I will tackle doc/decisions/4_decided/elektra_prefix.md with one PR per library (i.e. per src/libs/* folder in this PR). After that PR-series, the "gigantic but simplistic" PRs should be done and further PRs will be focused on updating a one or two functions at a time. (e.g. removing the automatic meta:/ from keyGetMeta and keySetMeta).

also extract base and pluginload object libraries

This changes many functions from internal.c into static inline since they are just wrappers with extra asserts. This change should improve the compiler's ability to perform optimizations, since it now always has insight into the functions. The static inline functions are only declared in internal headers. To make them available publically, we also re-export them via elektra-utility.so.

kodebach · 2023-06-02T18:02:00Z

@atmaxinger @hannes99 This PR might take a bit more time to review. The simple #include changes are all done and this PR is more focused on the library split and CMake updates. However, the CMake updates are all pretty similar, so it should still be quicker to review than 300 changed files might make it appear.

kodebach · 2023-06-04T16:48:03Z

jenkins build libelektra please

kodebach · 2023-06-05T12:21:46Z

jenkins build libelektra please

kodebach · 2023-06-06T20:12:21Z

jenkins build libelektra please

kodebach · 2023-06-13T13:56:44Z

@atmaxinger @flo91 Could one of you take a look at this PR?

- The package `texlive-most` is not available anymore, I've replaced it with `texlive-core` and `texlive-bin`

fix error in Cirrus CI build

- fixes failed build of refman.pdf with recent TeX Live versions - see issue ElektraInitiative#4966

atmaxinger · 2023-06-13T20:39:45Z

src/libs/kdb/symbols.map

+	elektraChangeTrackingGetContextFromPlugin;
+	elektraChangeTrackingCalculateDiff;
+
+	# FIXME: separate lib? part of merge?


I don't see why diff should be a part of merge TBH. A separate lib maybe, but on the other hand it's an integral part of KDB so IDK.

The change tracking part is integral to KDB, but the pure diff part could be used standalone (just with core), AFAIK. I thought of merge, since diff & merge are somewhat related concepts.

EDIT: Although I think merge depends on libgit, so its probably not even possible to put the diff stuff there.

atmaxinger · 2023-06-13T20:41:31Z

src/libs/utility/symbols.map

+	elektraMemDup;
+	elektraRealloc;
+	elektraStrCaseCmp;
+	elektraStrCmp;


Maybe a bit off-topic, but why exactly do we need our own implementation of strcmp?

The symbols.map files actually need to be cleanup when all this restructuring is done. elektraStrCmp isn't actually public anymore. It's now just an internal static inline function with extra asserts around standard strcmp.

libelektra/src/include/internal/utility/compare.h

Lines 40 to 45 in 3af13a2

static inline int elektraStrCmp (const char * s1, const char * s2)

{

ELEKTRA_ASSERT (s1 != NULL && s2 != NULL, "Got null pointer s1: %p s2: %p", VOID_CAST (s1), VOID_CAST (s2));

return strcmp (s1, s2);

}

This PR actually turns most of these "just a standard libc function with extra asserts" into internal static inline functions. For the ones that need to be public (elektraMalloc, etc.) we re-export them via libelektra-utility as normal functions too, but internally use the static inline versions. More complicated things like elektraFormat are still normal functions everywhere, but are linked statically (but unexported) into libelektra-core to avoid a dependency.

kodebach · 2023-06-13T20:53:35Z

jenkins build libelektra please

atmaxinger

Another great PR. 🚀

I left a few comments, but overall LGTM.
However, I'm not sure if it's a good idea to have the Latex package changes in this PR. I'd much rather see them fixed in master, and then rebase this branch to master again.

benchmarks/cmp.c

doc/KEYNAMES.md

doc/decisions/2_solutions_clear/simplify_api.md

doc/news/_preparation_next_release.md

src/CMakeLists.txt

kodebach · 2023-06-13T21:46:56Z

However, I'm not sure if it's a good idea to have the Latex package changes in this PR. I'd much rather see them fixed in master, and then rebase this branch to master again.

I cherry picked the commits (without change) from #4967, so if all goes well, they should disappear when this is eventually merged into master. 3ec05e1 and f3b783b might need to be applied to master too, I opened #4969.

Co-authored-by: Maximilian Irlinger <[email protected]>

kodebach · 2023-06-13T23:52:17Z

jenkins build libelektra please

This was referenced May 22, 2023

Move headers Part 2 #4940

Merged

Move headers Part 1 #4920

Merged

kodebach changed the base branch from master to new-repo-layout June 1, 2023 10:40

kodebach mentioned this pull request Jun 1, 2023

New repo layout #4956

Closed

kodebach force-pushed the move-headers-3 branch 6 times, most recently from d72d21a to 7a8afcd Compare June 1, 2023 14:48

kodebach added 12 commits June 2, 2023 19:41

cmake: severly reduce use of include_directories in global context

bd8099f

general: split elektra folder into core and kdb

4b3ba3d

also extract base and pluginload object libraries

doc: update details of library_split decision

b063fa1

types: split type system into separate library

78719b6

cmake: fix BUILD_FULL and BUILD_STATIC and cleanup cruft in the process

8e90877

doc: fix links

c855305

general: update includes after move

f56b0ee

general: fix new gcc warnings

679085b

general: fix formatting

b7e7377

doc: move decisions to 6_implemented

58387fd

doc: add release notes highlight

5dd0940

kodebach force-pushed the move-headers-3 branch from 7a8afcd to 5dd0940 Compare June 2, 2023 17:42

doc: fix formatting

844bac7

kodebach marked this pull request as ready for review June 2, 2023 18:00

kodebach requested review from atmaxinger and hannes99 June 2, 2023 18:00

doc: fix links

63da455

kodebach added 2 commits June 4, 2023 18:49

cmake: fix kdb lib built without shared

cc71104

benchmark: fix uninit bug

3fbba2a

ci: run more tests on ubuntu kinetic to diagnose packaging issue

b29778f

kodebach requested a review from flo91 June 13, 2023 13:55

kodebach force-pushed the move-headers-3 branch from c95960a to abafe92 Compare June 13, 2023 14:16

flo91 added 6 commits June 13, 2023 16:19

doc: update release notes

73b4bf7

CI: Update Dockerfile for Arch on Cirrus

a2fd515

- The package `texlive-most` is not available anymore, I've replaced it with `texlive-core` and `texlive-bin`

bindings: include <cstdint> in key.hpp for uint8_t

22f584d

fix error in Cirrus CI build

Cirrus: Add texlive-latexextra for varwidth.sty

061dfc2

Cirrus: Add texlive-fontsextra for stix.sty

4e5d5c3

Cirrus: Add texlive-science

b4afa92

kodebach force-pushed the move-headers-3 branch from abafe92 to ad75ba8 Compare June 13, 2023 14:21

flo91 and others added 2 commits June 13, 2023 16:24

doc: fix Doxygen for keyName function

2ea3fec

- fixes failed build of refman.pdf with recent TeX Live versions - see issue ElektraInitiative#4966

ci: fix cirrus clone script

f3b783b

kodebach force-pushed the move-headers-3 branch from ad75ba8 to f3b783b Compare June 13, 2023 14:24

kodebach added 2 commits June 13, 2023 20:02

ci: fix macos python issue

3ec05e1

toml: use elektraWriteArrayNumber instead of custom code

3af13a2

atmaxinger reviewed Jun 13, 2023

View reviewed changes

atmaxinger approved these changes Jun 13, 2023

View reviewed changes

kodebach mentioned this pull request Jun 13, 2023

CI fixes #4969

Merged

19 tasks

general: apply suggestions from code review

26537ec

Co-authored-by: Maximilian Irlinger <[email protected]>

kodebach merged commit aaacaf6 into ElektraInitiative:new-repo-layout Jun 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move headers Part 3 #4941

Move headers Part 3 #4941

kodebach commented May 22, 2023 •

edited

Loading

kodebach commented May 22, 2023

kodebach commented Jun 2, 2023

kodebach commented Jun 4, 2023

kodebach commented Jun 5, 2023

kodebach commented Jun 6, 2023

kodebach commented Jun 13, 2023

atmaxinger Jun 13, 2023

kodebach Jun 13, 2023 •

edited

Loading

atmaxinger Jun 13, 2023

kodebach Jun 13, 2023

kodebach commented Jun 13, 2023

atmaxinger left a comment

kodebach commented Jun 13, 2023

kodebach commented Jun 13, 2023

	static inline int elektraStrCmp (const char * s1, const char * s2)
	{
	ELEKTRA_ASSERT (s1 != NULL && s2 != NULL, "Got null pointer s1: %p s2: %p", VOID_CAST (s1), VOID_CAST (s2));

	return strcmp (s1, s2);
	}

Move headers Part 3 #4941

Move headers Part 3 #4941

Conversation

kodebach commented May 22, 2023 • edited Loading

What happens in this PR?

Hints for reviewers

Basics

Checklist

Review

Labels

kodebach commented May 22, 2023

kodebach commented Jun 2, 2023

kodebach commented Jun 4, 2023

kodebach commented Jun 5, 2023

kodebach commented Jun 6, 2023

kodebach commented Jun 13, 2023

atmaxinger Jun 13, 2023

Choose a reason for hiding this comment

kodebach Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

atmaxinger Jun 13, 2023

Choose a reason for hiding this comment

kodebach Jun 13, 2023

Choose a reason for hiding this comment

kodebach commented Jun 13, 2023

atmaxinger left a comment

Choose a reason for hiding this comment

kodebach commented Jun 13, 2023

kodebach commented Jun 13, 2023

kodebach commented May 22, 2023 •

edited

Loading

kodebach Jun 13, 2023 •

edited

Loading