-
Notifications
You must be signed in to change notification settings - Fork 123
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
[decisions] headers #4246
[decisions] headers #4246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Input before meeting.
2. Other things are truly private, but must be tested. | ||
This includes e.g. most other `split*` functions or the `elektraKeyName*` functions. | ||
|
||
Symbols belonging to this category should not appear in a public section of the `symbols.map` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these symbols at all? In theory unit tests could simply include the .c files (with static functions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO including .c files is a bad idea.
- It means that there will be a separate definition of the function in every file that includes the function. The performance aspect wouldn't matter for tests, but it would definitely break things like
static
variables (if we have those anywhere). - I think it's harder to define rules for when including
.c
files is okay and which files can be included where, than to just say "if you need to test a private function, make it non-static, add it to afortests
header and add it to thefortests
section of thesymbols.map
".
## Assumptions | ||
|
||
- There are different categories of "private": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very important point, actually it is a decision on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly would the decision part of the decision be? I'm just stating facts here. The assumption is that all symbols can be put into one of these four groups. But I'm not sure there is anything to decide here. The decision to use these four groups to build our headers is what this decision is about. Not sure, what you want to separate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably more standard to use the word "protected" for something that is "public" within Elektra's repository but private for the outside world (point 3).
The assumption is that all symbols can be put into one of these four groups.
Then write what you mean:
- There are different categories of "private": | |
- We assume that all parts of the API belong to one of the following visibility categories: |
Not sure, what you want to separate here.
The separate decision would be about which parts belong to which "visibility category". You give good examples here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm honestly I don't think we should do a seperate decision here. The examples that @kodebach listed serve illustrative purposes in this decision.
Creating decisions just for documenting where each function of each library should be placed is too much IMHO. That sounds very much like documentation for documentation's sake. Checking the placement is something that should be part of the PR review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we apply the suggestion by @markus2330 then we must also define 'public' and 'internal' visibilities in this section.
doc/decisions/header_include.md
Outdated
|
||
The rules for including headers are: | ||
|
||
- Including another header from the current directory is done with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't introduce such a rule. It is very hard to enforce it. And what is the benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very hard to enforce it.
Code review is always an option, but it also shouldn't be too hard to write a script that checks this, or add a custom query to our lgtm.com build job.
And what is the benefit?
- It clearly shows whether or not a header is part of the same library or not.
- You need to type less than. The alternative would be always having
<elektra/mylib/header.h>
. - Generally I'd say in C code the convention is
<>
is for external/system headers and""
is for local headers. So when I see a<>
I expect it to come from somewhere else. - When you use
""
your editor/IDE doesn't need to know about the correct include path. Since the header will be in the same directory the header can be found without extra setup. With<>
an editor/IDE that isn't setup explicitly, will just look in the system folders. So it might find a version of the header, but it could be the wrong one (installed vs. local source).
Ideally I'd like to use ""
for including all Elektra headers from within the code base. Then there can't be any confusion. But this would require careful coordination between the source tree and the install locations and also the use of ../
, so I made this compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I'd say in C code the convention is <> is for external/system headers and "" is for local headers.
This is agreed. But including system header files (also from other system header files) is something else then including local headers.
When you use "" your editor/IDE doesn't need to know about the correct include path.
This is wrong in general. ""
doesn't guarantee it is in the current directory (it might be from the include path) and we also shouldn't try to give this guarantee as it is hard to enforce. There is not really a way around configuring the IDE properly.
doc/decisions/header_include.md
Outdated
``` | ||
- Including a header from another library within the Elektra codebase, is done with: | ||
```c | ||
#include <elektra/otherlib/header.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 see also #817
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Headers are already installed into /usr/include/elektra
, but we don't properly use the <>
/""
right now. Which is why we need a custom include path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you try to say. Files installed to /usr/include
can be included via <>
without any include path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to clarify the relation to #817. The problem is not where the headers are installed, it is how we include them.
@markus2330 I added the results from the meeting today, please check if this can be merged already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep decisions short and to the point. Also make sure they are about important decisions within the architecture and framework of Elektra. I don't think that ""
vs. <>
includes can fulfill these points.
As an example of what I was aiming for with this decision, take a look at tree-sitter. They use a top-level Now obviously tree-sitter is only a single library unlike Elektra, but the ideas could be adapted.
For dependencies between public headers
AFAICT this approach really simplifies the include path config too. The include path would just point to the top-level |
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 |
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 |
I agree that this is useful. |
@markus2330 Can we agree on this approach?
AFAICT the only remaining issue in this scheme was
If we agree on this scheme, I will rewrite the decision documents so this PR can finally be merged. |
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
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.
Yes, so we need to keep the package-config files. This implication should be also mentioned in the decision. |
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+"\.\/'
I could probably make it do that, but a simple regex replace might be enough here. |
If you write such a script, I do not have objections against the new rules of header files. |
Sadly using 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 |
@markus2330 The decisions are now updated, please review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further discussion.
## Assumptions | ||
|
||
- There are different categories of "private": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably more standard to use the word "protected" for something that is "public" within Elektra's repository but private for the outside world (point 3).
The assumption is that all symbols can be put into one of these four groups.
Then write what you mean:
- There are different categories of "private": | |
- We assume that all parts of the API belong to one of the following visibility categories: |
Not sure, what you want to separate here.
The separate decision would be about which parts belong to which "visibility category". You give good examples here.
Will be installed as `<include-root>/elektra/foo.h`. | ||
- `src/include/elektra/foo/internal.h`: | ||
Contains the internal (i.e., non-stable) API of `libelektra-foo`. | ||
Will be installed as `<include-root>/elektra/foo/internal.h`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should install this file.
|
||
- `src/include/elektra/foo/internal.h`: | ||
Declares the internal (i.e., non-stable) API of `libelektra-foo`, by including `#include <elektra/foo/*.h>`. | ||
Will be installed as `<include-root>/elektra/internal/foo.h`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it is okay to have internal header files (to be used within other modules of Elektra) but they should not be installed.
### Plugins | ||
|
||
Plugins do not declare their API via header files. | ||
Their headers are never installed and can be named any way the developer wants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Plugins do not declare their API via header files. | ||
Their headers are never installed and can be named any way the developer wants. | ||
|
||
### Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info also needs to be in doc/TESTING.md
|
||
## Rationale | ||
|
||
- This structure makes the `#include`s simple and works nicely with our directory structure. | ||
- Having a uniform naming convention simplifies things, both for developers writing Elektra and those using Elektra. | ||
- Requiring libraries must either be fully modularized (main headers are only `#includes`) or completely monolithic, avoids the situation where a library has some parts in `public.h` and other parts in extra headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
A modularized library `bar` may have these headers (covering categories 3 & 4 from above): | ||
|
||
- `src/include/elektra/foo/public.h`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the word public here. What about "foo/foo.h" to include everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a massive fan of the repetition, but I guess it's better than public
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered "all" and "everything".
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 ( |
@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:
With this distinction I agree, that the
A bit of explanation why I think this is better than excluding whole libraries from semver guarantees:
|
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. |
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. |
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 #4246 (comment). If so, I will update the decision files in the PR. |
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. |
Yes, I know you want to merge 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 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 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".
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. |
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.
Perfect, this is what I want it to be, too! 👍
Lets try to remove the philosophical stuff. 🪚 |
Okay, so we do agree after all. I'll update the files in this PR, when I have some spare time. |
@markus2330 Can this be merged now? If so, I will resolve the merge conflict. |
I already approved before: Please write my reservation in the notes of header_include.md and rebase. 🚀 |
I made more changes after the review, that's why I re-requested the review.
The decision already states in multiple places that a tool will check the new rules. If you want more, please make a concrete suggestion.
I'll do that when I know the merge is imminent, because the conflict will just reappear when another decision PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last changes LGTM
Please rebase after #4635 |
Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>
c4ba680
to
fded0c9
Compare
@markus2330 rebased, please merge when CI is done |
There are several broken links. Please check CI first before mentioning me. |
Sorry, didn't have time to wait for the CI earlier and I hoped nothing would fail, since this is just text change and it worked before the rebase. However, it seems I forgot to update some links. Not sure, though what the issue on Jenkins is... Seems like some Docker image can't be found. Definitely not something from this PR. |
Seems like https://wiki.gnome.org/ is down. But that is clearly a temporary problem, so I think we can ignore the failed Link Checker. |
@markus2330 I think this can be merged |
@atmaxinger is your comment is an approval? |
Yes |
More decision in addition to #4243