2.1 only: add tests/scripts/doxygen.sh and fix make apidoc

[gilles-peskine-arm]

Add tests/scripts/doxygen.sh like in 2.7 and development. This lets us keep our CI scripts uniform. Fix #2010.

Fix the doxygen configuration to work with modern Doxygen versions without warnings.

make apidoc now generates the documentation for the current configuration. Run scripts/apidoc_full.sh to generate the full documentation. This aligns the behavior with Mbed TLS 2.2+ and permits parallel builds (#390 #391).

This is a backport of a branch that was merged in 2.2.2 at 9a3ee57 without a Github pull request.

Additionally, there is a backport of the commit “Look for documentation only in specific directories” from #2031, which was not included in #2033 because it interacts with other changes in this PR.

We also debated backporting #1284 for uniformity with the other maintained branches but this is a change in behavior and not a bug fix in any way.

[Patater]

retest

[andresag01]

@gilles-peskine-arm: Thanks for the PR, I think the changes look good.

Before approving I have a quick question: Why are there some differences in the backported changes between the 2.1 branch and development? For example, I think the 2.1 branch assumes that doxygen is run from the project root while development assumes that it is run from the doxygen directory. I do not know why this is the case, but perhaps its ideal to keep them in sync?

[gilles-peskine-arm]

@andresag01 That's a good question. The change in directory was made in #1284. I'd initially preferred to keep the changes minimal and only backport what had been done for 2.2. However I hadn't paid attention that #1284 is already present in 2.7 (it was merged just before the 2.7 release), so 2.1 remains the odd man out. Backports should minimize behavior changes, but this patch already changes the behavior of make apidoc (full config → current config), and the Doxyfile gave warnings with modern Doxygen versions. Arguments against backporting: it's a bigger behavior change, and the Doxyfile warnings could be ignored. So should #1284 be backported as well? @andresag01 @mpg

[andresag01]

@gilles-peskine-arm: I agree that generally we should aim to maintain behaviour in the LTS. However, the change in #1284 seems minor as it is just regarding the place where the docs are generated. It would be a different discussion if we were talking about a change in the API or ABI of the code. I am not sure about the warnings. Are they also being ignored in the development and 2.7 branches? If this is the case, then I do not think we should be too concerned about that. In summary, I would prefer it to be backported as well.

[mpg]

@gilles-peskine-arm @andresag01 I'm sorry, can you clarify what the behaviour changes are currently, and what additional behaviour change would be introduced by backporting #1284 too? By "the place where the docs are generated", do you mean (a) what the working directory needs to be when invoking the script, or (b) the final place where the docs are going to be found after generation, or (c) something else?

[gilles-peskine-arm]

The following changes were made in 2.2 (via a PR-less branch), and this PR backports them:

• Before: modern versions of Doxygen emitted a warning about unknown settings in the doxyfile. After: no warnings. This change does not affect the Doxygen output.
• Before: make apidoc generates the documentation for the full configuration. After: make apidoc generates the documentation for the current configuration.
• Before: make apidoc lib fails in parallel builds because it temporarily moves config.h. After: make -j apidoc lib works.
• Before: there is no tests/scripts/doxygen.sh so we can't indiscriminately call it in CI jobs. After: tests/scripts/doxygen.sh works so we can call it from CI jobs.

The following change was made in 2.7 (via #1284) and I'm considering backporting it:

• Before: if you run Doxygen manually with the Doxyfile (as opposed to calling make apidoc), you have to run it from the top-level directory. After: if you run Doxygen manually, you have to run it from the doxygen subdirectory. (So @mpg it's (a) only.)

[mpg]

@gilles-peskine-arm Thanks for that very clear summary!

The changes that this PR already backports are obviously fine and desirable. In my opinion, the change introduced by #1284 is also acceptable for the 2.1 branch, as the behaviour when invoking make apidoc is unchanged. (It's debatable whether running doxygen manually is part of our guaranteed-stable API, but in practice even in the unlikely case someone was doing it and this change is a problem for them, there are easy fixes.) I'd argue that since this change is acceptable, it's also desirable, in order to get closer alignment between branches.

[mpg]

I think Andres and me both think #1284 should be backported, and you don't seem to disagree, so I'm adding "needs work" and will review once that additional part has been backported.

[mpg]

This looks like a proper backport of the set of doxygen-related things we want to backport.

[simonbutcher]

I'm really open to discussion or debate, but I'm really not sold on changing the targets in the Makefile on a maintained branch. I see no strong or compelling reason to do it.

[mpg]

The maintained branches are supposed to be bug-fixes only, and test enhancements. This exceeds that.

I agree about reverting the backport of #1284 for that reason, but I think the other behaviour change was necessary to fix a bug, hence its inclusion in a maintained branch was justified. The bug is that make -j all apidoc will fail (with an error in the best case, or silently producing inconsistent results), and I'm not sure there is a simple way to fix that without also changing the behaviour of the target.

I think it was pjbakker's intention that the documentation be generated this way, and I think we should leave it as is.

I checked the history and I don't think the behaviour was really Paul's intention. That change was introduced in 2.2.1 and backported in 2.1.4, so we would merely be reverting to what the behaviour was from 2.1.0 to 2.1.3. For some reason we backported the faulty commit but forgot to backport its reversal (that is, until this PR).

I think that strengthens my previous point: not only is this fixing a bug, it's fixing a bug that we introduced in a maintained branch (while trying to fix another one, but going the wrong way about it).

(By the way, if we followed your advice back then and tried harder to avoid changing behaviour in maintained branches, we probably would have been forced to fix the warnings another way and would have avoided introducing this bug in the first place. So I think your care is justified - I just happen to disagree about this particular instance.)

[gilles-peskine-arm]

The current version of the PR as I write (which I'm archiving to https://github.com/gilles-peskine-arm/mbedtls/tree/doxygen-fixes-2.1-make_apidoc_is_not_full) makes two changes which are at the border between enhancements and bugs. They don't reconcile the behavior between the implementation and the documentation because we didn't document the intended behavior — but by the same token they don't break documented behavior.

• We didn't explicitly promise that parallel builds work. But it's a natural expectation, and more and more so over time.
• We didn't explicitly promise that make apidoc generates the documentation according to the current configuration as opposed to the full documentation of every possible feature. But it's a natural expectation that the sole build target that generates documentation builds the documentation that corresponds to the code, and not the documentation for a different configuration.

As Manuel notes, both expectations were met until 2.1.3, which is an argument for fulfilling them again. On the other hand, they've been broken on the 2.1 branch for 2½ years and nobody has complained that I could find, which is an argument for leaving things as they are.

I have a preference for fixing the arguably-bugs, but it isn't a strong preference. I have a strong preference for getting tests/scripts/doxygen.sh in one way or another.

[gilles-peskine-arm]

A branch that preserves the 2.1.4+ behavior (make apidoc generates the full documentation) is at https://github.com/gilles-peskine-arm/mbedtls/tree/doxygen-fixes-2.1-no_target_change

[simonbutcher]

Ok, I can accept @mpg's point. We're just reverting it back to the way it was done in mbedtls-2.1.3. There's a danger of over debating this, and what is effectively a small change to something that doesn't change the library itself. I was trying to stick to a point of principle here, because since 2.1.3, which was our first LTS and maintained branch, we've learnt a lot about user's expectations on maintaining the interfaces, and I don't think we'd make the same decisions again - and maintaining consistency of build targets and supported toolchains is important on such branches.

Therefore I accept the change.

[gilles-peskine-arm]

Since you've both approved the PR, I take it this is approved for design. The CI job that was breaking has successfully run tests/scripts/doxygen.sh (https://jenkins-internal.mbed.com/job/mbedtls-restricted-pr/412/). So this is presumably ready for merge.

[simonbutcher]

retest

[simonbutcher]

retest

[simonbutcher]

retest

[simonbutcher]

CI passes! Yay! Labelling as 'Ready to Merge'.