libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.71k stars 1.8k forks source link

Issues with SDL3 man pages #7514

Closed madebr closed 11 months ago

madebr commented 1 year ago

Since cd7a3f8af5b683970003d260b0739ba5059eb40a, the CMake build system installs man pages when a perl interpreter is available. This issue tracks some remaining issues:

icculus commented 1 year ago

What is the minimum required perl version to execute build-scripts/wikiheaders.pl?

Probably any version since the late 1980's. :)

It does rely on Perl's Text::Wrap module, which is an external dependency, but I don't know how one checks for this sort of thing in Perl, beyond running perl -e 'use Text::Wrap;' and seeing if the exit code is zero.

icculus commented 1 year ago

(Garbled manpages are definitely my bug, and are just a matter of me seeing a page with bad output and fixing whatever bug caused it. I'll take a look at the linked example.)

icculus commented 1 year ago

Probably any version since the late 1980's. :)

Oh, I can see that older perls don't like the hash-dereference nonsense that @sezero ran into. Let's just say you need 5.22 for it to work. It's not life-and-death if the manpages can't build, especially since it needs a checkout of the wiki in any case.

The garbled manpage thing is because we're turning *** markdown (bold+italic) into `.BI' (bold+italic) sections in the manpage...but apparently these just look like garbage in general.

Macro: .BI text

Set its arguments alternately in bold face and italic, without a space between the arguments.

(Without a space?! Wild.)

So we'll just make these things bold and call it a day.

sezero commented 1 year ago

Probably any version since the late 1980's. :)

Oh, I can see that older perls don't like the hash-dereference nonsense that @sezero ran into. Let's just say you need 5.22 for it to work.

Applying the following patch makes it work for me with perl-5.10.1 (which includes hash dereference fixes along with fixes from old commit b38a5ba) and it still works with 5.26.2 too. @icculus: OK to apply?

diff --git a/build-scripts/wikiheaders.pl b/build-scripts/wikiheaders.pl
index b49a3fd..bb09faa 100755
--- a/build-scripts/wikiheaders.pl
+++ b/build-scripts/wikiheaders.pl
@@ -529,8 +529,8 @@ if (defined $readmesubdir) {
 }

 opendir(DH, $incpath) or die("Can't opendir '$incpath': $!\n");
-while (readdir(DH)) {
-    my $dent = $_;
+while (my $d = readdir(DH)) {
+    my $dent = $d;
     next if not $dent =~ /$selectheaderregex/;  # just selected headers.
     open(FH, '<', "$incpath/$dent") or die("Can't open '$incpath/$dent': $!\n");

@@ -678,8 +678,8 @@ my %wikitypes = ();  # contains string of wiki page extension, like $wikitypes{"
 my %wikifuncs = ();  # contains references to hash of strings, each string being the full contents of a section of a wiki page, like $wikifuncs{"SDL_OpenAudio"}{"Remarks"}.
 my %wikisectionorder = ();   # contains references to array, each array item being a key to a wikipage section in the correct order, like $wikisectionorder{"SDL_OpenAudio"}[2] == 'Remarks'
 opendir(DH, $wikipath) or die("Can't opendir '$wikipath': $!\n");
-while (readdir(DH)) {
-    my $dent = $_;
+while (my $d = readdir(DH)) {
+    my $dent = $d;
     my $type = '';
     if ($dent =~ /\.(md|mediawiki)\Z/) {
         $type = $1;
@@ -816,14 +816,14 @@ if ($copy_direction == 1) {  # --copy-to-headers
         next if not defined $wikifuncs{$fn};  # don't have a page for that function, skip it.
         my $wikitype = $wikitypes{$fn};
         my $sectionsref = $wikifuncs{$fn};
-        my $remarks = %$sectionsref{'Remarks'};
-        my $params = %$sectionsref{'Function Parameters'};
-        my $returns = %$sectionsref{'Return Value'};
-        my $threadsafety = %$sectionsref{'Thread Safety'};
-        my $version = %$sectionsref{'Version'};
-        my $related = %$sectionsref{'Related Functions'};
-        my $deprecated = %$sectionsref{'Deprecated'};
-        my $brief = %$sectionsref{'[Brief]'};
+        my $remarks = $sectionsref->{'Remarks'};
+        my $params = $sectionsref->{'Function Parameters'};
+        my $returns = $sectionsref->{'Return Value'};
+        my $threadsafety = $sectionsref->{'Thread Safety'};
+        my $version = $sectionsref->{'Version'};
+        my $related = $sectionsref->{'Related Functions'};
+        my $deprecated = $sectionsref->{'Deprecated'};
+        my $brief = $sectionsref->{'[Brief]'};
         my $addblank = 0;
         my $str = '';

@@ -1386,8 +1386,8 @@ if ($copy_direction == 1) {  # --copy-to-headers
         if ( -d $readmepath ) {
             mkdir($wikireadmepath);  # just in case
             opendir(DH, $readmepath) or die("Can't opendir '$readmepath': $!\n");
-            while (readdir(DH)) {
-                my $dent = $_;
+            while (my $d = readdir(DH)) {
+                my $dent = $d;
                 if ($dent =~ /\AREADME\-(.*?\.md)\Z/) {  # we only bridge Markdown files here.
                     my $wikifname = $1;
                     next if $wikifname eq 'FrontPage.md';
@@ -1398,8 +1398,8 @@ if ($copy_direction == 1) {  # --copy-to-headers

             my @pages = ();
             opendir(DH, $wikireadmepath) or die("Can't opendir '$wikireadmepath': $!\n");
-            while (readdir(DH)) {
-                my $dent = $_;
+            while (my $d = readdir(DH)) {
+                my $dent = $d;
                 if ($dent =~ /\A(.*?)\.(mediawiki|md)\Z/) {
                     my $wikiname = $1;
                     next if $wikiname eq 'FrontPage';
@@ -1463,15 +1463,15 @@ if ($copy_direction == 1) {  # --copy-to-headers
         next if not defined $wikifuncs{$fn};  # don't have a page for that function, skip it.
         my $wikitype = $wikitypes{$fn};
         my $sectionsref = $wikifuncs{$fn};
-        my $remarks = %$sectionsref{'Remarks'};
-        my $params = %$sectionsref{'Function Parameters'};
-        my $returns = %$sectionsref{'Return Value'};
-        my $version = %$sectionsref{'Version'};
-        my $threadsafety = %$sectionsref{'Thread Safety'};
-        my $related = %$sectionsref{'Related Functions'};
-        my $examples = %$sectionsref{'Code Examples'};
-        my $deprecated = %$sectionsref{'Deprecated'};
-        my $brief = %$sectionsref{'[Brief]'};
+        my $remarks = $sectionsref->{'Remarks'};
+        my $params = $sectionsref->{'Function Parameters'};
+        my $returns = $sectionsref->{'Return Value'};
+        my $version = $sectionsref->{'Version'};
+        my $threadsafety = $sectionsref->{'Thread Safety'};
+        my $related = $sectionsref->{'Related Functions'};
+        my $examples = $sectionsref->{'Code Examples'};
+        my $deprecated = $sectionsref->{'Deprecated'};
+        my $brief = $sectionsref->{'[Brief]'};
         my $decl = $headerdecls{$fn};
         my $str = '';
icculus commented 1 year ago

Yeah, go ahead and commit that.

slouken commented 1 year ago

Patch applied as 1f095b9ce (I need it here, too!)

smcv commented 1 year ago

Debian's Lintian tool reports this, which might be helpful:

W: libsdl3-doc: groff-message 26: warning: macro 'flags'' not defined (possibly missing space after 'fl') [usr/share/man/man3/SDL_CreatePopupWindow.3.gz:1]
N: 
N:   A manual page provoked warnings or errors from the man program. Here are some common ones:
N:   
N:   "cannot adjust" or "can't break" are issues with paragraph filling. They are usually related to long lines.
N:   Justifying text on the left hand side can help with adjustments. Hyphenation can help with breaks.
N:   
N:   For more information, please see "Manipulating Filling and Adjusting" and "Manipulating Hyphenation" in the
N:   Groff manual (see info groff).
N:   
N:   "can't find numbered character" usually means that the input was in a national legacy encoding. The warning
N:   means that some characters were dropped. Please use escapes such as \[:a] as described on the groff_char manual
N:   page.
N:   
N:   Other common warnings are formatting typos. String arguments to .IP require quotes. Usually, some text is lost
N:   or mangled. See the groff_man (or groff_mdoc if using mdoc) manual page for details on macros.
N:   
N:   The check for manual pages uses the --warnings option to man to catch common problems, like a . or a ' at the
N:   beginning of a line as literal text. They are interpreted as Groff commands. Just reformat the paragraph so the
N:   characters are not at the beginning of a line. You can also add a zero-width space (\&) in front of them.
N:   
N:   Aside from overrides, warnings can be disabled with the .warn directive. Please see "Debugging" in the Groff
N:   manual.
N:   
N:   You can see the warnings yourself by running the command used by Lintian:
N:   
N:       LC_ALL=C.UTF-8 MANROFFSEQ='' MANWIDTH=80 \
N:           man --warnings -E UTF-8 -l -Tutf8 -Z <file> >/dev/null
N: 
N:   Please refer to the groff_man(7) manual page and the groff_mdoc(7) manual page for details.
W: libsdl3-doc: groff-message 31: warning: macro ':' not defined [usr/share/man/man3/SDL_CreatePopupWindow.3.gz:2]
N:
W: libsdl3-doc: groff-message 33: warning: macro 's' not defined [usr/share/man/man3/SDL_RWtell.3.gz:1]
N:
W: libsdl3-doc: groff-message ... use "--tag-display-limit 0" to see all (or pipe to a file/program)
N:
I: libsdl3-doc: acute-accent-in-manual-page [usr/share/man/man3/SDL_GetBasePath.3.gz:61]
N: 
N:   This manual page uses the \' groff sequence. Usually, the intent is to generate an apostrophe, but that sequence
N:   actually renders as an acute accent.
N:   
N:   For an apostrophe or a single closing quote, use plain '. For single opening quote, i.e. a straight downward
N:   line ' like the one used in shell commands, use '\(aq'.
N:   
N:   In case this tag was emitted for the second half of a '\\' sequence, this is indeed no acute accent, but still
N:   wrong: A literal backslash should be written \e in the groff format, i.e. a '\\' sequence needs to be changed to
N:   '\e' which also won't trigger this tag.
N: 
N:   Please refer to Bug#554897, Bug#507673, and Bug#966803 for details.
madebr commented 1 year ago

@icculus Does the perl script have a stable interface? I'd like to install the perl script such that the SDL3 satellite libraries can generate man pages too.

icculus commented 1 year ago

It has changed as new features are added, but I'm willing to commit to future command lines adjustments being additions with no changes or removals.

Under the chance that this might not be Perl forever, though: should we remove the .pl extension?

madebr commented 1 year ago

I can probably wrap your script in a cmake function, and make that one visible. With some abstraction, we wouldn't care about the implementation language, just about the exposed features.

icculus commented 1 year ago

Debian's Lintian tool reports this, which might be helpful:

I totally missed this comment before, I'll look this over soon!

madebr commented 1 year ago

~The .wikiheaders-options file contains perl specific regexes. These should probably be translated into something understandable by both perl and an alternative language (e.g. python).~ Scrap that, I haven't tested this yet.

Python understands the regexes, so everything is fine. Sorry about the noise.

madebr commented 1 year ago

For SDL_image, the scripts emits warnings for every function, e.g. IMG_Init defined in the headers but not the wiki https://github.com/libsdl-org/SDL_image/actions/runs/5384619672/jobs/9772751875?pr=368#step:12:29 Are my changes to .wikiheaders-options invalid?

icculus commented 1 year ago

Hmmm...I think the wikisubdir option is wrong (should be SDL3_image now, not SDL_image, I think). SDL_image doesn't get a lot of documentation updates, perhaps it's not you, but this has been broken awhile and we never noticed?

madebr commented 1 year ago

I think the issue is with warn_about_missing = 1. When running the script for the first time, no .md files are present in the wiki dir. Is it a problem if a symbol is documented in the SDL headers, but not (yet) on the wiki? The script is essentially repopulating an empty wiki.

Disabling the following lines lets the warnings disappear: https://github.com/libsdl-org/SDL/blob/937f5d0059664c37d0dffcaf94422afd310407ea/build-scripts/wikiheaders.pl#L800-L805 l

smcv commented 1 year ago

Under the chance that this might not be Perl forever, though: should we remove the .pl extension?

If it's intended to become part of the interface of SDL's development files (like sdl2-config used to be), then please remove the extension and give it a name that is namespaced to SDL 3, so that it won't collide with a similar tool in SDL 2, SDL 4 or a non-SDL-related package. It could be installed as /usr/bin/sdl3-wikiheaders for example.

(In Debian world, I'd put it in the libsdl3-dev package, or Fedora packagers would put it in SDL3-devel or whatever.)

The .wikiheaders-options file contains perl specific regexes

There is a family of compatible regular expression syntaxes based on Perl 5, which includes Perl 5 itself, Python re, libpcre, GNU grep -P, and others (I think JavaScript RegExp is also Perl-5-compatible). The distinguishing feature of this family of syntaxes is that putting a backslash before a non-alphanumeric character (like \() always disables its special behaviour (if any) and turns it into a literal.

I'd suggest documenting wikiheaders configuration as requiring PCRE-compatible regexes: that way, you're free to implement it in Perl 5, Python, anything that wraps libpcre (for example GLib's GRegex), and so on. The cost is that you should stick to the common subset used by all of those implementations, rather than using some of the more advanced features added in recent Perl or Python versions.

Notable regular expression families that are not PCRE-compatible include POSIX basic regular expressions (sed, grep), POSIX extended regular expressions (grep -E), traditional vi, vim (not the same as vi!), Google re2, and Raku (formerly Perl 6).

madebr commented 1 year ago

https://github.com/libsdl-org/SDL/commit/937f5d0059664c37d0dffcaf94422afd310407ea installs the perl script to lib/cmake/SDL3/wikiheaders.pl. It is only used from within the CMake function(s) in lib/cmake/SDL3/sdlmanpages.cmake. The only way to access it is though cmake:

find_package(SDL3)
SDL_generate_manpages(
  SYMBOL "IMG_Init"
)

(The SYMBOL argument is a somewhat broken way to work around currently missing dependency management)

icculus commented 1 year ago

I'd suggest documenting wikiheaders configuration as requiring PCRE-compatible regexes:

This is probably a good time to say that this tool was written to run on exactly one computer, is extremely quirky in some ways, and its promotion to a part of the public SDL3 ecosystem is making me more nervous the more we talk about this.

icculus commented 1 year ago

When running the script for the first time, no .md files are present in the wiki dir.

We moved SDL over from mediawiki to markdown, and probably didn't for the satellite libraries...maybe that's the culprit?

slouken commented 1 year ago

I think this is breaking the build here for me:

Can't opendir '/opt/buildbot/buildworker/sdl3_osx/build/build/tmp/arm64/docs/wiki': No such file or directory
smcv commented 1 year ago

This is probably a good time to say that this tool was written to run on exactly one computer, is extremely quirky in some ways, and its promotion to a part of the public SDL3 ecosystem is making me more nervous the more we talk about this.

Copying the script into the satellite libraries, and periodically updating their copy, might well be a safer option than declaring it to be a stable interface.

slouken commented 1 year ago

This is probably a good time to say that this tool was written to run on exactly one computer, is extremely quirky in some ways, and its promotion to a part of the public SDL3 ecosystem is making me more nervous the more we talk about this.

Copying the script into the satellite libraries, and periodically updating their copy, might well be a safer option than declaring it to be a stable interface.

Yes, let's do that. That approach has worked well with the autotools scripts that we had with SDL2.

madebr commented 1 year ago

Okay, the script is not getting installed anymore. About the build bot error, can you provide more info? For building the man pages of SDL3, nothing changed.

icculus commented 1 year ago

About the build bot error, can you provide more info?

I'll look at this soon, it's likely my bug.

slouken commented 1 year ago

About the build bot error, can you provide more info?

I'll look at this soon, it's likely my bug.

Nope, @madebr fixed it, the directory just needed to be created.