Skip to content
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

treewide: Use deprecations in code rather than comments #18650

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Sep 26, 2022

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 #18561.

It already has the macros and Doxygen stuff, it lacks

  • change of all @deprecated to DEPRECATED
  • 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-declarations if 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 RIOT-OS/riotdocker#189

@github-actions github-actions bot added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: doc Area: Documentation Area: drivers Area: Device drivers labels Sep 26, 2022
@chrysn chrysn added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for CI update State: The PR requires an Update to CI to be performed first and removed Area: drivers Area: Device drivers labels Sep 26, 2022
@benpicco
Copy link
Contributor

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)

@maribu
Copy link
Member

maribu commented Sep 26, 2022

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.

@chrysn
Copy link
Member Author

chrysn commented Sep 27, 2022

Yes, that's the plan.


Docs now look as good as it gets: flashpage_first_free looks like that
Screenshot 2022-09-27 at 07-33-29 Flash page driver
(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).

This keeps deprecations at the warning level during regular development,
but enforces that no deprecated items are used in internal code, unless
the test explicitly allows that.
@github-actions github-actions bot added Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: drivers Area: Device drivers Area: tests Area: tests and testing framework labels Oct 3, 2022
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 3, 2022
@chrysn
Copy link
Member Author

chrysn commented Oct 3, 2022

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=0 to allow testing things until they are eventually removed.
  • Murdock sets FORBID_DEPRECATED=1 as 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)?

@github-actions github-actions bot added the Area: OTA Area: Over-the-air updates label Oct 3, 2022
@github-actions github-actions bot added the Area: sys Area: System label Oct 3, 2022
This change also serves as an example of how an item can still be used
in code that needs to use it for some time after the deprecation.
@chrysn
Copy link
Member Author

chrysn commented Oct 3, 2022

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).

@chrysn chrysn requested review from bergzand and dylad as code owners October 3, 2022 12:28
@github-actions github-actions bot added the Area: USB Area: Universal Serial Bus label Oct 3, 2022
@chrysn chrysn removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 3, 2022
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 3, 2022
@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Oct 3, 2022
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 3, 2022
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. If possible, I would prefer to split the lines to match the maximum char limit (e.g. see suggestions), so that the CI is happy. But I do not insist.

core/lib/include/kernel_defines.h Outdated Show resolved Hide resolved
drivers/include/periph/usbdev.h Outdated Show resolved Hide resolved
drivers/include/periph/flashpage.h Outdated Show resolved Hide resolved
sys/include/suit/transport/coap.h Outdated Show resolved Hide resolved
sys/include/suit/transport/coap.h Outdated Show resolved Hide resolved
drivers/include/periph/flashpage.h Outdated Show resolved Hide resolved
@chrysn
Copy link
Member Author

chrysn commented Oct 4, 2022

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.

Rename DEPRECATED to RIOT_DEPRECATED_x and split between enums and
functions to gain flexibility in where the attribute goes precisely (as
the documentation goes somewhere different)

This also word-wraps all the texts.
@MrKevinWeiss
Copy link
Contributor

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.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also handle compiler abstractions? Maybe check for LLVM?

I suppose everything is gcc first and only portions of llvm are supported so maybe that would be OK since it is more about warnings rather then code execution.

@maribu
Copy link
Member

maribu commented May 15, 2023

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.

@@ -280,6 +280,61 @@ extern "C" {

/* end{code-style-ignore} */

/** @brief Declare a function as deprecated
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
*
* @param reason Reason for removal
* @param eol Expected end of life
* @param migration Migration path (e.g. API to use instead)
*

Maybe enforce consistency and completeness of the doc by having explicit arguments for the important details. That would also decrease the itch to use fancy formatting.

Beware: Indent levels are not correct, did this on a phone.

@maribu
Copy link
Member

maribu commented Oct 8, 2024

@chrysn Ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: OTA Area: Over-the-air updates Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms State: waiting for CI update State: The PR requires an Update to CI to be performed first Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants