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

docs: ascii version of manpage without nroff #13047

Closed
wants to merge 9 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 4, 2024

create ascii version of the manpage without nroff

  • build src/tool_hugegelp.c from the ascii manpage
  • the newly generated tool_hugehelp is 3K lines shorter and 50K smaller
  • move the the manpage and the ascii version build to docs/cmdline-opts
  • remove all use of nroff from the build process
  • should make the build entirely reproducible (by avoiding nroff)

The ascii version of the manpage:

  • is built with gen.pl, just like the manpage is
  • has a right-justified column making the appearance similar to the previous version
  • uses a 4-space indent per level (instead of the old version's 7)
  • does not do hyphenation of words (which nroff does)

history

We first made the curl build use nroff for building the hugehelp file in December 1998, for curl 5.2.

curl -M sample

This is what the top of new version looks like in my terminal:

curl-txt

@github-actions github-actions bot added the tests label Mar 4, 2024
@bagder bagder marked this pull request as ready for review March 5, 2024 08:02
@bagder bagder changed the title docs: generate an ascii version of the man page docs: ascii version of manpage without nroff Mar 5, 2024
@bagder
Copy link
Member Author

bagder commented Mar 5, 2024

I'm trying hard to figure out what exactly it is that fails in the cmake + MSVC jobs on AppVeyor... 😕 And nowhere else.

@github-actions github-actions bot added the CI Continuous Integration label Mar 5, 2024
@bagder
Copy link
Member Author

bagder commented Mar 5, 2024

The error message from cd2nroff: Failed to open CURLOPT_SOCKS5_GSSPI_SERVICE.md : No such file or directory is puzzling, as it has lost an A in the middle of the file name.

The command is invoked by cmake where it provides a long list of *.md file names for cd2nroff to convert to nroff.

... without using nroff.

- build src/tool_hugegelp.c from the ascii manpage
- move the the manpage and the ascii version build to docs/cmdline-opts
- remove all use of nroff from the build process

The ascii version of the manpage:

- is built with gen.pl, just like the manpage is
- has a right-justified column making the appearance similar to the
  previous version
- uses a 4-space indent per level
- does not do hyphenation of words (which nroff does)
Use a plain array and puts() every line, also allows us to provide the
strings without newlines.

- merge blank lines into the next one as a prefixed newline.
- turn eight consecutive spaces into a tab (since they can only be on the
  left side of text)
@bagder
Copy link
Member Author

bagder commented Mar 6, 2024

The latest test in this PR that seems to have made the build work again is a partial revert of 2620aa9 that @vszakats did. It seems as if the very long command line from cmake is problematic? What I can't explain is if why that suddenly would become a problem in this PR, nor why it does not fail more reliably.

The command line in question is over 10,000 characters long and Windows is often said to have an 8K limit (?). The change in this PR converts that make to single script invokes for each conversion.

@vszakats
Copy link
Member

vszakats commented Mar 6, 2024

The Windows command-line limit used to be 8K for XP and earlier, but has been 32K since. Also why dropping chars from the middle? why now indeed? [correction: it's 8K for XP and later for cmd.exe and 32K for CreateProcess().]

To avoid that, maybe cd2nroff could be extended to accept a list in file (e.g. @inputs.txt)? That could be generated by CMake dynamically. Going back to single invocations is significantly slower.

@bagder
Copy link
Member Author

bagder commented Mar 6, 2024

To avoid that, maybe cd2nroff could be extended to accept a list in file (e.g. @inputs.txt)? That could be generated by CMake dynamically. Going back to single invocations is significantly slower.

I can't see why not. Generating them all in a single invoke makes it ineffective dependency wise as then it will rebuild them all even if only one is changed, but especially on Windows generating them all like that will likely be noticeably faster when all of them need to be rendered.

I will however not try to do this within this PR as it seems unrelated. We could of course argue that my revert is also unrelated, but I could not get this PR to build clean on appveyor without that!

@bagder bagder closed this in f03c856 Mar 6, 2024
bagder added a commit that referenced this pull request Mar 6, 2024
Use a plain array and puts() every line, also allows us to provide the
strings without ending newlines.

- merge blank lines into the next one as a prefixed newline.
- turn eight consecutive spaces into a tab (since they can only be on the
  left side of text)
- the newly generated tool_hugehelp is 3K lines shorter and 50K smaller
- modifies the top logo layout a little by reducing the indent

Closes #13047
@vszakats
Copy link
Member

vszakats commented Mar 6, 2024

So far so good: https://github.com/curl/curl-for-win/actions/runs/8176058376

I'll re-compare the generated vs distributed --manual content when the daily snapshot tarball gets its update tomorrow.

vszakats added a commit to curl/curl-for-win that referenced this pull request Mar 6, 2024
nroff no longer used with v8.7.0:
curl/curl#13047

On a local machine, building the manual takes a long time relative
to other build steps:

(in seconds)
configure:           8
build curl manual:  17
build curl/libcurl:  4
post-process:        5
launching wine:     11

Likely also noticable in CI. Esp. on aggregate.

For now I keep the local hack to use the pre-built manual when
available. This helps with CI times.

We can improve this later by:

1. a dev options to disable the manual fully (for local builds,
   which often use a Git sandbox without the pre-built manual)

2. making manual generation faster

3. fixup cmake to allow enabling the `--manual` option without
   rebuilding the manual if a pre-built one exists, _without_ our
   local hack of copying files around.
@bagder bagder deleted the bagder/asciipage branch March 11, 2024 12:36
vszakats added a commit to vszakats/curl that referenced this pull request Mar 27, 2024
This time limit the number of files per command to avoid exceeding
limitations of certain OS/shell envs.

Such known env is Windows with the `cmd.exe` shell, which features an
8K limit to this day.

Allowlisting `UNIX` to have no limit and using a limit of 200 for other
envs to be safe. If there is a way to detect `cmd.exe` and/or we know
which precise envs are sensitive to this, we can tweak these conditions
further.

Even with the low limit, this patch reduces external commands by 200x,
making builds much faster.

Ref: curl#13047
Ref: curl#12762 2620aa9

Closes #xxxxx
michaelforney added a commit to michaelforney/curl that referenced this pull request Mar 28, 2024
docs was added to SUBDIRS in curl#13047, so having it in install-data-hook
is now redundant and causes make to install docs twice.

After removing it, both conditional recipes are identical, so drop
the conditional.
vszakats added a commit that referenced this pull request Apr 4, 2024
This time limit the number of files per command to avoid exceeding
limitations of certain OS/shell envs.

Such known env is Windows with the `cmd.exe` shell, which features an
8K command-line length limit to this day.

Allowlisting `UNIX` to have no limit and using a limit of 200 for other
envs to be safe. If there is a way to detect `cmd.exe` and/or we know
which precise envs are sensitive to this, we can tweak these conditions
further.

Even with the low limit, this patch reduces external commands by 200x,
making builds much faster.

Ref: #12762 2620aa9 (initial)
Ref: #13047 f03c856 (revert)

Reviewed-by: Daniel Stenberg
Closes #13207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants