RIOT-OS
treewide: Use deprecations in code rather than comments
This is a replacement PR for #18565 because that had been made to the wrong branch and GitHub closed it irrevertably when I tried to clean up.
The relevant parts of that PR are replicated here:
Contribution description
This executes https://github.com/RIOT-OS/RIOT/issues/18561.
It already has the macros and Doxygen stuff, it lacks
- change of all
@deprecatedtoDEPRECATED - CI time rejection of non-code deprecation (unsure if possible, maybe we still need it for module level deprecations)
- Removal of deprecated functions in the code base, or annotation with
-Wno-error=deprecated-declarationsif we still want to test the deprecated functions.
Testing procedure
Right now:
- See how CI starts breaking because we -Werror and the one function that got changed is still used in tests.
- Look at Doxygen output on the changed function.
- [edit: added] Also try building an example of a harder deprecation with a newer C standard where the actual C version (and not the GCC proprietary one) is used:
$ CFLAGS=-std=c2x make -C examples/usbus_minimal BOARD=samr21-xpro
[edit: or the more forgiving gnu2x, given that native has some trouble with being precisely conformant]
Long term:
- Try using a deprecated function, see things fail.
@maribu said:
If you throw in an -Wno-error=deprecated-declarations, the warning should not be escalated to an error. We may however want to still escalate it to an error with RIOT_CI_BUILD=1 to avoid in-tree users sneaking back in.
--
@chrysn responded:
On the severity of errors, I think a good path would be to have Makefiles of ALLOW_DEPRECATED ?= 1, make Murdock set ALLOW_DEPRECATED = 0, and have some tests (those that explicitly include deprecated functions because we don't want these to accidentally break) set ALLOW_DEPRECATED = 1.
On the CI side, Murdock went well because it duly rejected the tests that used the deprecated function (even though, as above, these should later be exempt).
Sadly, doxygen output breaks completely with the current doxygen version -- deprecation text gets moved into functions list, but the function itself is not present any more at all, and the next function is somehow also missing ... putting this in as "waiting for CI update" because the next Ubuntu hopefully has a better Doxygen. (Things looked good with the 1.9.4 I have here).
This is now blocked by https://github.com/RIOT-OS/riotdocker/pull/189
Can we also add CFLAGS += -Wno-error=deprecated-declarations so users don't have to fix those immediately? (Which is the whole point of the deprecation period)
I think this is @chrysn's plan anyway. But for the CI runs it will be an error (except for some apps explicitly testing for backward compatibility), so that no new in-tree users sneak in.
Yes, that's the plan.
Docs now look as good as it gets: flashpage_first_free looks like that
(i.e. no links, and quotes around the text). If that's not good enough, I think we'd need to resort to double-tracking -- having text both in the docs and in the deprecation, and keeping them in sync.
Reviewers, is that good enough for you? (I'd like to know before I do the treewide edits).
I've made the intended handling of deprecations explicit in the latest commit. It introduces a FORBID_DEPRECATED variable similar to WERROR. The code that maps the variable to the CFLAGS peeks into WERROR as well in order not to flood the CFLAGS with redundant input.
The new behavior is as follows, with the strongest rules on top:
- Particular tests, such as the one changed, can force
FORBID_DEPRECATED=0to allow testing things until they are eventually removed. - Murdock sets
FORBID_DEPRECATED=1as a general preference. - In absence of any settings, the use of deprecated items is allowed (and produces a compiler warning)
Do you have any preferences as to whether the treewide edit (moving as many deprecations as possible into C attributes) would be in here or a follow-up (with this one merely setting the stage)?
While I'm adding selected examples, I've started looking into how to get The Big List of things that should be deprecated in C rather than in documentation. Right now my best expression is
$ rg @deprecated -g '!*.txt' -g '!vendor'
because doc.txt files and vendor headers often include Doxygen deprecations that can't (practically) be moved to C.
I also found that there's no straightforward way to deprecated enums as a whole -- a DEPRECATED("...") line before an enum { does create the right documentation, but to make the individual values raise, there'd need to be a deprecation marker after every single value (eg. SPI_NODEV DEPRECATED("...") = -ENXIO,). That's impractical right now (both from a maintenance and a documentation side), so I'll leave those aside for now. (Also, that concrete deprecation would trigger quite a lot of errors during CI, given how often it still occurs in the code). Worse goes for groups of defines, which AFAICT can not be recognizably deprecated at all.
One more case of "code needs to declare when it uses deprecated stuff" I've found is that when a deprecated enum value (such as USBDEV_EVENT_TR_FAIL) is C-level deprecated, implementing a case for it (which may easily be necessary during the deprecation period) also trips. This can be remedied by pushing pragmas (which contrary to the literal value works with all our supported compilers); an example is now pushed. If we don't like that, we'd probably just relax on how deprecations become hard in CI (either per example/test, or generally).
In the end, replacing the existing documentation deprecations with C ones is likely to be a highly manual process, with many case-by-case decisions; so I'm leaning toward moving that to a later commit (except for the few items done so far, which'd serve as examples).
I'll need to rename this due to collisions -- cpu/stellaris_common/include/vendor/ do some ifndef DEPRECATED, and that could conflict subtly.
Anyway, I'm not fully convinced that the deprecation with attributes works well around enum items, so my next change series will change this to:
RIOT_DEPRECATED_FUNCTION("text")to go before function declarations,RIOT_DEPRECATED_VARIANT(ENU_ITEM, "text")to go around enum items
and apply line breaking.
Really nice, this is a good solution for C code, I wonder if we can expand this or at least reuse the marco semantics to handle deprecation of tools or build system info?
We would also need to publicise it to all maintainers.
Should we also handle compiler abstractions? Maybe check for LLVM?
Note that clang also defines __GNUC__ and is mostly a drop-in replacement for GCC. The deprecated attribute is also supported by it: https://clang.llvm.org/docs/AttributeReference.html#deprecated
Since we don't support any compiler but GCC and clang/LLVM and there are no plans to do so, IMO this is just fine as it is.