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.

Sign up for GitHub

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

Jump to bottom

treewide: Use deprecations in code rather than comments #18565

Closed
wants to merge 2 commits into from
Closed

treewide: Use deprecations in code rather than comments #18565

wants to merge 2 commits into from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Sep 6, 2022

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.

Long term:

  • Try using a deprecated function, see things fail.

@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 6, 2022
@chrysn chrysn added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed Area: doc Area: Documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Sep 6, 2022
chrysn
chrysn commented Sep 6, 2022
View reviewed changes
core/lib/include/kernel_defines.h
*
* On modern (C2x) compilers, this expands to the C++11 style attribute
* `[[deprecated(reason)]]`; otherwise to the GNUism
* `__((attribute(deprecated))`, or is ignored.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
* `__((attribute(deprecated))`, or is ignored.
* `__attribute_((deprecated))`, or is ignored.

Copy link
Member

Choose a reason for hiding this comment

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

should be even __attribute__, shouldn't it?

@chrysn chrysn added CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 6, 2022
@maribu
Copy link
Member

maribu commented Sep 6, 2022

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
Copy link
Member Author

chrysn commented Sep 7, 2022

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

Doxygen did not fail but just failed to upload due to timeouts, retrying.

@chrysn
Copy link
Member Author

chrysn commented Sep 7, 2022

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

@chrysn chrysn added the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Sep 7, 2022
@chrysn chrysn mentioned this pull request Sep 7, 2022
Merged
@chrysn chrysn mentioned this pull request Sep 21, 2022
Open
@chrysn chrysn deleted the branch RIOT-OS:deprecate-in-c September 26, 2022 11:07
@chrysn chrysn closed this Sep 26, 2022
@chrysn chrysn mentioned this pull request Sep 26, 2022
Open
@chrysn
Copy link
Member Author

chrysn commented Sep 26, 2022

There was some screw-up with branches -- see #18650 for a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu left review comments

Assignees
No one assigned
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrysn @maribu