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: Use the correct Doxygen Logo File Type in the Footer #21274

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Mar 6, 2025

Contribution description

Apparently Doxygen changed the default file type from PNG to SVG at some point and the footer was never updated, leading to the ugly "image not found" symbol in the footer.
The SVG file is already present and accessible on the webserver ( https://doc.riot-os.org/doxygen.svg ) but not in the footer.html.

Current state:
image

image

With this PR:
image

Testing procedure

Check that the generated documentation contains the logo in the footer.

Apparently the old doxygen version (1.9.1) in the CI container has some issues displaying the SVG, because there is no size specification:
image

In newer Doxygen versions (1.12.0), the CSS settings for the footer were extended:
image

So you have test it locally, wait for the CI container to be updated or just trust me :D

Issues/PRs references

None.

@crasbe crasbe requested review from aabadie and jia200x as code owners March 6, 2025 15:36
@github-actions github-actions bot added the Area: doc Area: Documentation label Mar 6, 2025
@crasbe crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 6, 2025
@riot-ci
Copy link

riot-ci commented Mar 6, 2025

Murdock results

✔️ PASSED

2657394 doc: use the correct logo file type in the footer

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

Artifacts

@mguetschow
Copy link
Contributor

So you have test it locally, wait for the CI container to be updated or just trust me :D

... Or add that same CSS rule to our RIOT tree to fix it for all users?

@crasbe
Copy link
Contributor Author

crasbe commented Mar 7, 2025

So you have test it locally, wait for the CI container to be updated or just trust me :D

... Or add that same CSS rule to our RIOT tree to fix it for all users?

I'm a bit torn here, because the old Doxygen versions are not supported anyway due to various bugs, which motivated #21277.

The footer-logo-width CSS parameter was introduced in 1.9.5, which was released in mid 2022: https://github.com/doxygen/doxygen/blob/Release_1_9_5/templates/html/doxygen.css#L447-L451

This might be a personal thing, but I'd rather have the users/developers update their Doxygen version that working around one bug while still having many other bugs.

@mguetschow
Copy link
Contributor

I'm a bit torn here, because the old Doxygen versions are not supported anyway due to various bugs, which motivated #21277.

Sure I agree, but reality is that those versions are still around on developer machines. #21273 would continue to work around the markdown issue on local machines even when we have our CI and website doxygens updated. I don't think adding another CSS rule with a comment on it being a workaround would harm.

@crasbe
Copy link
Contributor Author

crasbe commented Mar 7, 2025

Unfortunately I don't really have the CSS skills to develop a solution that won't interfere with future Doxygen updates.

Considering 22.04 LTS is the last LTS distro to use Doxygen 1.9.1 and 24.04 LTS uses 1.9.8 ( https://launchpad.net/ubuntu/noble/+source/doxygen ) which doesn't have the same issue anymore, I'm not convinced to include a hack that might cause issues in the future when it's long forgotten.
Likewise, I'd be in favor to remove the doc.md hack, once 1.14.0 is out and integrated into the CI system, for the same reason.

But I'm open for a third opinion (or a proposal for the CSS rule).

@mguetschow
Copy link
Contributor

Summoning @kfessel here since he was advocating for keeping local user installations functional here #21273 (review).

Maybe an alternative solution would be to allow users to use the doxygen version in riotbuild when we got that one up to date at some point, probably with the fix proposed here: #21106 (comment)

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I do not care for that logo and probably many people building local wont either

@crasbe crasbe force-pushed the pr/fix_doxygen_logo branch from 9893d21 to 2657394 Compare March 10, 2025 12:14
Copy link
Contributor

@mguetschow mguetschow 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 now, let's ship it :)

@mguetschow mguetschow enabled auto-merge March 10, 2025 13:19
@mguetschow mguetschow added this pull request to the merge queue Mar 10, 2025
Merged via the queue into RIOT-OS:master with commit 8dae0df Mar 10, 2025
26 checks passed
@crasbe crasbe deleted the pr/fix_doxygen_logo branch March 10, 2025 13:28
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants