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

doc: remove documentation on deprecated RIOT_FILE_* macros #21320

Closed
wants to merge 3 commits into from

Conversation

mguetschow
Copy link
Contributor

Contribution description

#21170 added documentation about RIOT_FILE_RELATIVE and RIOT_FILE_NOPATH which were already considered obsolete in #18936. Instead of going through the deprecation hoops I'd propose to just revert these commits before the next release.

Issues/PRs references

Reverts #21170.

@github-actions github-actions bot added the Area: doc Area: Documentation label Mar 25, 2025
@mguetschow mguetschow changed the title Remove documentation on deprecated RIOT_FILE_* macros doc: remove documentation on deprecated RIOT_FILE_* macros Mar 25, 2025
@mguetschow mguetschow requested review from miri64 and removed request for jia200x and aabadie March 25, 2025 17:32
@mguetschow mguetschow added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Mar 25, 2025
@riot-ci
Copy link

riot-ci commented Mar 25, 2025

Murdock results

✔️ PASSED

bffa112 doxygen: remove deprecated RIOT_FILE_RELATIVE

Success Failures Total Runtime
1 0 1 01m:28s

Artifacts

@crasbe
Copy link
Contributor

crasbe commented Mar 26, 2025

That change is good, because with the current CFLAGS in makefiles/cflags.inc.mk, the documentation is incorrect:
@see __FILE__ for absolute filenames that also work with *.h files

vs.

# Don't include absolute path in __FILE__ macro
OPTIONAL_CFLAGS += -fmacro-prefix-map=$(RIOTBASE)/=

Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

Small nitpick: There is an excess whitespace in one of your commits in front of the "doc".

@crasbe crasbe added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 26, 2025
@mguetschow mguetschow force-pushed the doc-file-macros-revert branch from 2a424b4 to bffa112 Compare March 27, 2025 13:25
@crasbe
Copy link
Contributor

crasbe commented Mar 28, 2025

From my side, this is good to go, but I'd prefer to leave the decision to @miri64.

You can probably prepare a PR already to remove RIOT_FILE_* entirely, but merging has to wait until #21110 is merged (if you don't want to force @Ollrogge to rebase 😅 ).

@Ollrogge
Copy link
Member

From my side, this is good to go, but I'd prefer to leave the decision to @miri64.

You can probably prepare a PR already to remove RIOT_FILE_* entirely, but merging has to wait until #21110 is merged (if you don't want to force @Ollrogge to rebase 😅 ).

all good, I dont mind rebasing :)

miri64
miri64 previously approved these changes Apr 1, 2025
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

If we can use standard macros without spilling the absolute path 👍 from my side.

@miri64 miri64 enabled auto-merge April 1, 2025 06:46
@miri64 miri64 added this pull request to the merge queue Apr 1, 2025
@miri64 miri64 removed this pull request from the merge queue due to a manual request Apr 1, 2025
@miri64
Copy link
Member

miri64 commented Apr 1, 2025

But wait... should we maybe go through the official deprecation cycle first. So deprecate them for removal after the 2025.10 release?

@mguetschow
Copy link
Contributor Author

But wait... should we maybe go through the official deprecation cycle first. So deprecate them for removal after the 2025.10 release?

Well, since they've never been documented (and not used throughout the codebase in the last years), I'd argue this is not part of the public API and therefore doesn't qualify for the deprecation process. In particular, the documentation from #21170 hasn't made it into any release yet.

@miri64
Copy link
Member

miri64 commented Apr 2, 2025

Well, since they've never been documented (and not used throughout the codebase in the last years), I'd argue this is not part of the public API and therefore doesn't qualify for the deprecation process.

Only because something is not documented, doesn't mean it is not used. It was a feature that has been there for years and though hidden, people might still rely on it. Is there any hurry to get it out? If no, let's go the official path. If yes, the reasons should be clearly stated in OP.

In particular, the documentation from #21170 hasn't made it into any release yet.

Our documentation is not tagged to releases, so I find this sentence a somewhat strange claim.

@mguetschow
Copy link
Contributor Author

Is there any hurry to get it out? If no, let's go the official path

Alright, closing in favor of #21345

@mguetschow mguetschow closed this Apr 2, 2025
@mguetschow mguetschow deleted the doc-file-macros-revert branch April 2, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants