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

ICU-22922 ICU4C produce search doxygen doc by default #3408

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

mihnita
Copy link
Contributor

@mihnita mihnita commented Feb 21, 2025

The instruction for publishing the icu4c doc (generated with doxygen) use the doc-searchengine make target.
(see https://unicode-org.github.io/icu/processes/release/tasks/docs.html#icu4c-3)

The result is that the documentation generated has a search box top-right:
https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/

Requires JavaScript, but it is very useful, and this is what we published for a about 5 years (ICU 68.1).
(see the history of the apidoc/released/icu4c/search folder).

But by default we still produce the non-search version of the doc.

That is what we create with make dist, what we install locally, and what we publish in the GitHub released artifacts.
(see for example https://github.com/unicode-org/icu/releases/download/release-76-1/icu4c-76_1-docs.zip)

That is less useful.

It is also inconsistent: what I get if I build / install locally or download from release does not match what we publish on the web.
And also means we can't automate as well (we need to build manually to publish the web doc instead of using the official release files).

This change would make everything consistent, with the search version of the C/C++ documentation being default everywhere.


An alternative would be to rename the make targets:
doc => doc-nosearch and doc-searchengine => doc

Checklist

  • Required: Issue filed: ICU-22922
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@markusicu
Copy link
Member

The changes seem fine, but the icu4c-docs-build CI check fails.

@mihnita
Copy link
Contributor Author

mihnita commented Feb 21, 2025

I checked, and it is caused by the following warnings:

warning: Tag 'TCL_SUBST' at line 251 of file '-' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'COLS_IN_ALPHA_INDEX' at line 1076 of file '-' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'PERL_PATH' at line 2159 of file '-' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'MSCGEN_PATH' at line 2181 of file '-' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"

They are detected and caused to fail by this part of the command in icu4c.yml:
... && ( ! grep -P 'warning:(?! .* file .?Doxyfile)' doxygen.log )

Which is a good protection, I'm not complaining.
But it does not exist in the make file, only in the GitHub action.

It fails with the same warnings when doing make doc
So I don't know why it didn't fail until now.

My guess is that at some point doxygen was updated to a newer version.

Removing the flags does not affect the output (I checked).

I can remove them by hand, but running doxygen -u updates the whole file and adds some flags that seem useful.
So I'm investigating that as an alternative, and I will update the PR soon.

@mihnita mihnita force-pushed the mihai_doxygen_search branch from 840675e to b4aab50 Compare February 21, 2025 17:43
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/Doxyfile.in is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@mihnita
Copy link
Contributor Author

mihnita commented Feb 21, 2025

I've updated Doxyfile.in with all the changes produces by doxygen -u
I diff-ed the results, and there are no differences.

If you feel uneasy about it so close to release I can just keep the original Doxyfile.in and remove the offending flags.
And do the full update after release.

Let me know.

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

lgtm, let's do it, and thanks!

@mihnita
Copy link
Contributor Author

mihnita commented Feb 21, 2025

Weird, the tests passed 16h ago.

But the action does a sudo apt-get -y install doxygen
So it does not even depend on GitHub updating images.
It depends on whatever ubuntu decides to do, what packages they update and when.

@mihnita mihnita merged commit 6cc1618 into unicode-org:main Feb 21, 2025
102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants