[decisions] headers

[kodebach]

More decision in addition to #4243

[kodebach]

@markus2330 I added the results from the meeting today, please check if this can be merged already

[kodebach]

As an example of what I was aiming for with this decision, take a look at tree-sitter. They use a top-level include folder for the public headers, which has the same structure as will be installed. All the non-public headers live directly in the src folder next to the source files. For #include the use <> with system headers, "tree_sitter/..." for public headers and "./..." for private headers.

Now obviously tree-sitter is only a single library unlike Elektra, but the ideas could be adapted.

1. Public headers live in include/elektra/[libname]
2. Private headers live next to the source files in e.g. src/libs/core-c/ or src/libs/kdb.

For dependencies between public headers #include <elektra/[libname]/header.h> is always used, for other files following rules apply:

1. Including system/external headers uses #include <header.h>
2. Including private headers from the current library uses #include "./header.h"
3. Including public headers from the current or another library uses #include <elektra/[libname]/header.h>
4. Including private from another library is not allowed.

AFAICT this approach really simplifies the include path config too. The include path would just point to the top-level include folder and that's it.

[markus2330]

I am not convinced that there is really a problem that needs to be solved: I never had a problem that a wrong header file gets included. We definitely have a problem with installed header files (there are several kdb.h floating around and it would be great if #include <elektra/kdb.h> just works for applications without any include path hurdles. So I would concentrate on this problem, and if this is fixed we can also do internal changes. What do you think?

[kodebach]

Changing the way the headers are stored in the repo would make fixing the include path much simpler. The goal would be that the headers are stored in the same layout as when installed. That means we just need to add a single directory to the include path (in our CMake) to mimic the installed behaviour.

For the #includes: If we go with the solution proposed above, we'd essentially have the rule "always use <> unless the target is a private and in the same folder". The advantage here would be that only headers that are public and get installed will be in added to the include path. So accidentally including something that isn't installed (I think we had that happen once a few years ago), isn't possible, because it won't compile.

[markus2330]

The goal would be that the headers are stored in the same layout as when installed.

I agree that this is useful.

[kodebach]

@markus2330 Can we agree on this approach?

1. Public headers live in include/elektra/[libname]
2. Private headers live next to the source files in e.g. src/libs/core-c/ or src/libs/kdb.

For dependencies between public headers #include <elektra/[libname]/header.h> is always used, for other files following rules apply:

1. Including system/external headers uses #include <header.h>
2. Including private headers from the current library uses #include "./header.h"
3. Including public headers from the current or another library uses #include <elektra/[libname]/header.h>
4. Including private from another library is not allowed.

AFAICT the only remaining issue in this scheme was #include "./header.h". A bit of further explanation for this:

• I would enforce the ./ prefix to make abundantly clear that this is header only present in the local directory.
• Since it may only be used for private headers (which will not be installed), these #includes can only appear in source (.c) files or in headers that are themselves private.
• Together with the other rules, this scheme should allow a very simple configuration of the include path.
  • When Elektra is installed in a default location like /usr no configuration is needed. The headers will be in e.g. /usr/include/elektra/foo/foo.h and /usr/include will on the default include path. So #include <elektra/foo/foo.h> will just work.
  • When Elektra is installed in a non-default location like /home/user/stuff, just one directory has to be added to the include path (e.g. /home/user/stuff/include).
  • When building Elektra the include path is pointed at src/include. This directory will mirror the installed layout (e.g. src/include/elektra/foo/foo.h). Because we use #include "./header.h", we don't need to add extra include paths for private non-installed headers that live outside of src/include.

If we agree on this scheme, I will rewrite the decision documents so this PR can finally be merged.

[markus2330]

I would enforce the ./ prefix to make abundantly clear that this is header only present in the local directory.

My only objection is: how can we check and enforce such a rule? If we don't find any way, it is probably a dead and confusing rule. Such rules should not exist, as then also important rules (e.g. which header files to install where, thread-safety etc.) might not be read or not taken serious.

If you your automatic reformatter changes "./" and <> accordingly, then: why not, I don't have opinions about it. I would allow anything that works. To be more clear what I find worthwhile to actually check in reviews and tests (which is a lot of effort and can only be done for user-facing decisions):

Since it may only be used for private headers (which will not be installed), these #includes can only appear in source (.c) files or in headers that are themselves private. Together with the other rules, this scheme should allow a very simple configuration of the include path. When Elektra is installed in a default location like /usr no configuration is needed. The headers will be in e.g. /usr/include/elektra/foo/foo.h and /usr/include will on the default include path. So #include <elektra/foo/foo.h> will just work.

Here I agree 100%. This we should have and is the main goal of the decision. Implication: We must test inclusion of all header files without include path.

   When Elektra is installed in a non-default location like /home/user/stuff, just one directory has to be added to the include path (e.g. /home/user/stuff/include).

Yes, so we need to keep the package-config files. This implication should be also mentioned in the decision.

[kodebach]

how can we check and enforce such a rule?

Writing a script (maybe using clang-query) to run as a test case and in CI should be easy enough. As a first step, even this could work (although the false positive rate may be high)

# find #include "..." lines, within those exclude #include "./ lines
grep -E '^\s*#include\s+".+"' | grep -vE '^\s*#include\s+"\.\/'

If you your automatic reformatter changes "./" and <> accordingly

I could probably make it do that, but a simple regex replace might be enough here.

[markus2330]

Writing a script (maybe using clang-query) to run as a test case and in CI should be easy enough.

If you write such a script, I do not have objections against the new rules of header files.

[kodebach]

Sadly using clang-query doesn't work, since the Clang AST doesn't contain any nodes for preprocessor directives. But I checked against a tree-sitter query (tree-sitter's grammar has nodes for preprocessor directives), and found that the simple grep command from above is actually accurate.

So we can probably just use something like

find -type f -name "*.c" -print0 | xargs -0 grep -E '^\s*#include\s+".+"' | grep -vE '.*:\s*#include\s+"\.\/"'

At least until find a false positive. At that point we could add a whitelisting mechanism (e.g. adding a // ignore include comment at the end of the false positive line).

[kodebach]

@markus2330 The decisions are now updated, please review again.

[markus2330]

At least until find a false positive. At that point we could add a whitelisting mechanism (e.g. adding a // ignore include comment at the end of the false positive line).

We should avoid further whitelisting mechanisms. Wouldn't it be possible to write it differently to not trigger a false positive?

[kodebach]

We should avoid further whitelisting mechanisms. Wouldn't it be possible to write it differently to not trigger a false positive?

AFAICT with our formatting rules there shouldn't be any false positive (#includes are always at the start of a line). In a quick search I didn't find the exact rules of when the preprocessor detects an #include. If we know the exact rules (shouldn't be to complicated), we could of course write a script that detects all #includes.

[kodebach]

@markus2330 Let's move the discussion about "unstable" API from #4245 to this PR, since it also affects the header structure.

I have thought about it some more and I came to the conclusion that we shouldn't use the term "unstable" anymore. It is just causing confusion. Actually we should split what I used the term "unstable" for into two categories:

1. "internal": These APIs are technically public (i.e. exported symbols), but not meant for consumption out side of libelektra-* libraries. They exist for communication between libraries.
2. "experimental": These APIs are public and meant for consumption by external users, but they are not fully stable yet. None of the semantic versioning guarantees apply. There may be breaking changes even in a minor version change.

With this distinction I agree, that the internal headers should not be installed. However, I'd actually change the setup of src/include a bit to make this more obvious.

• src/include/internal/foo.h or src/include/internal/foo/header.h is for internal headers. These are not installed. Inside the repo they can in theory be included everywhere as e.g. #include <internal/foo.h>, since the include path points to src/include. To make sure we don't use these in an installed header, I would extend the script that checks #include "./..." to also ensure that #include <internal/... doesn't appear in src/include/elektra. Another advantage of moving internal headers out of src/include/elektra is during installation we simply copy all of src/include/elektra.
• src/include/elektra/experimental/foo.h or src/include/elektra/experimental/foo/header.h is for experimental headers. These are installed (since they are in src/include/elektra). Theoretically they can also be included anywhere as e.g. #include <elektra/experimental/foo.h>. Here the script would also need to be extended to make sure there is no #include <elektra/experimental/... in src/include/elektra (except for src/include/elektra/experimental).

Note: The third category of APIs is "stable" APIs in src/include/elektra (outside of src/include/elektra/experimental/). Therefore I use non-stable to refer to "internal" and "experimental" headers together.

A bit of explanation why I think this is better than excluding whole libraries from semver guarantees:

1. When a function moves from being experimental to stable (without API change), it remains in the same shared object. That means application don't need to be recompiled/relinked. Therefore, such a change would not be a breaking change and could be done in a minor version update. External source code probably needs to be updated for the new header (unless it is already included), but I think that's acceptable.
2. One advantage of having stable and non-stable libraries is that you can do e.g. ldd /usr/bin/kdb to check if an application uses non-stable libraries. But if we really consider that a feature, a stable library could never depend on a non-stable one, even if the stable library would abstract over all the non-stable APIs.
3. Even if all libraries are stable, you can't actually be sure that an application isn't using something that falls outside our semver guarantees. All the "internal" APIs are technically public and accessible to any app/library that knows the functions signatures (e.g. by copying the header from our repo). So the "use ldd /usr/bin/kdb to check for non-stable libs" idea isn't actually reliable.

[markus2330]

I think it is better time-management-wise if we continue the discussion about experimental libraries when we know which of our libraries will stay experimental even within 1.0. Maybe we can get all functions in the important libraries non-experimental, and I think everyone agrees that this would be the best.

[kodebach]

Maybe we can get all functions in the important libraries non-experimental, and I think everyone agrees that this would be the best.

Yes, but I'd like to finish this PR at some point... So if we can at least agree on the part about "internal" headers that would be nice.

Then I'd update the decisions and just mention that the decision must be updated before we can have "experimental" APIs, i.e. APIs that are not stable but should still be public. In other words, anything that isn't moved to an "internal" header would be public stable API as soon as 1.0 releases.

[markus2330]

So if we can at least agree on the part about "internal" headers that would be nice.

What is missing? The PR doesn't seem updated.

In other words, anything that isn't moved to an "internal" header would be public stable API as soon as 1.0 releases.

:+1:

[kodebach]

What is missing? The PR doesn't seem updated.

I wanted to know, if you agree with the ideas about "internal" headers from this comment https://github.com/ElektraInitiative/libelektra/pull/4246#issuecomment-1249185666. If so, I will update the decision files in the PR.

[markus2330]

I already said I am not a fan of individual symbols that are excluded from stability guarantees. I agree it might be necessary if we are unable to refactor everything nicely (e.g. in src/include/kdbprivate.h are many public symbols just for tests) but I don't see why we should write a complicated rule-framework for something which is actually not something we want.

It is important that decisions focus on something. It is political and not technical if decisions about headers try to punch holes through totally unrelated library conventions.

Furthermore, urgent now is merging new-backend. I don't see a correlation of these decisions and the work on new-backend.

[kodebach]

Furthermore, urgent now is merging new-backend. I don't see a correlation of these decisions and the work on new-backend.

Yes, I know you want to merge new-backend. But I don't think it's ready and I don't think I can do much to make it ready. At least not without trampling over the work others are trying to do.

The reason why I asked, is because I thought "Can we agree on ...?" is a simple yes/no question. And in fact I thought we already agreed and just wanted clarification. Clearly that's not the case. I asked, because it would have been very quick for me to update the PR and the could be merged and we'd have one less thing to worry about...

To get back to the issue at hand:

I already said I am not a fan of individual symbols that are excluded from stability guarantees. I agree it might be necessary if we are unable to refactor everything nicely (e.g. in src/include/kdbprivate.h are many public symbols just for tests) but I don't see why we should write a complicated rule-framework for something which is actually not something we want.

I have no idea what you are talking about.... Please don't even try to explain, it will not help the discussion.

Please just answer this very basic yes/no question: Do you agree that we need (at least) public headers which will be included in packages like libelektra-dev, and also internal headers, which exist and can be used in the repo, but will not be included in packages?

That is all I wanted to know, because the decision in this PR currently does not reflect this correctly. In the current file, a header is either included in packages or it is restricted to a single library. Ignore all the ideas about experimental/unstable headers that will be installed, I'll remove all that and replace it with something like "if we need an installed, but not stable API for 1.0, we need to make a new decision".

It is important that decisions focus on something. It is political and not technical if decisions about headers try to punch holes through totally unrelated library conventions.

Again no idea where this came from... This is a very technical decision. It is about where in the repo headers live, what headers are included in packages, what headers can be included in which files, etc. In a more general sense, this is about API guarantees which might be a bit more philosophical but is still very important, because it has real impact on users of Elektra.

[markus2330]

Yes, there must be header files that are internal to our repo, including but not limited to our struct definitions. It was wrong to install them, as this brings expectations that cannot and should not be fulfilled.

This is a very technical decision.

Perfect, this is what I want it to be, too! :+1:

might be a bit more philosophical

Lets try to remove the philosophical stuff. :carpentry_saw:

[kodebach]

Okay, so we do agree after all. I'll update the files in this PR, when I have some spare time.

[kodebach]

@markus2330 I updated the PR

[kodebach]

Who wants to be second reviewer?

No idea who would be best, but you probably need to tag someone or we need to bring it up in the next meeting. Otherwise, probably nobody will respond.

[atmaxinger]

Looks good to me 👍

[kodebach]

@markus2330 Can this be merged now? If so, I will resolve the merge conflict.

[markus2330]

I already approved before: Please write my reservation in the notes of header_include.md and rebase. :rocket:

[kodebach]

I already approved before

I made more changes after the review, that's why I re-requested the review.

Please write my reservation in the notes of header_include.md

The decision already states in multiple places that a tool will check the new rules. If you want more, please make a concrete suggestion.

rebase

I'll do that when I know the merge is imminent, because the conflict will just reappear when another decision PR is merged.

[markus2330]

Please rebase after #4635

[kodebach]

@markus2330 rebased, please merge when CI is done