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

Man page installation and distribution #12921

Closed
michaelforney opened this issue Feb 10, 2024 · 7 comments
Closed

Man page installation and distribution #12921

michaelforney opened this issue Feb 10, 2024 · 7 comments

Comments

@michaelforney
Copy link
Contributor

michaelforney commented Feb 10, 2024

I did this

Recently there's been a lot of changes around man pages concerning generation, installation, distribution, and build-by-default.

First, here's my understanding of some automake variables used in curl:

  • man_MANS: Man pages listed here will get built by default and installed, but not distributed
  • dist_man_MANS: Man pages listed here will get built by default, installed, and distributed.
  • noinst_man_MANS: This doesn't actually do anything. I don't see any reference to it in automake or the generated Makefile.
  • nodist_MANS: This doesn't do anything either. Looking at automake source, there is nodist_man_MANS, which is the same as man_MANS since man pages aren't distributed by default (see https://www.gnu.org/software/automake/manual/html_node/Man-Pages.html). The comment in the source says "We handle nodist_ for uniformity. man pages aren't distributed by default so it isn't actually very important".
  • EXTRA_DIST: Files here are distributed, but not installed or built by the default target.

Here's a summary of the recent history of a few man pages:

  • Before 8.6.0, mk-ca-bundle.1 was not generated, but it was distributed since it was in noinst_man_MANS, which was present in EXTRA_DIST.

    • 8.6.0 included eefcc1b, which added it to man_MANS, making it get installed and built by default. Since noinst_man_MANS was dropped from EXTRA_DIST, it no longer got distributed.
    • a911f4f removed it from man_MANS, so it was no longer installed or built by default. The only reference left to mk-ca-bundle.1 at this point was through noinst_man_MANS, a bogus variable not used by anything, and CLEANFILES.
    • 755b31d added it to dist_man_MANS, making it get installed once again, distributed, and built by default. This is the current state.

    It seems to me that the intent is for it to be built by default and distributed, but not installed. I think the way to do this is to remove mk-ca-bundle.1 from dist_man_MANS, add it to EXTRA_DIST to distribute it, and add a dependency all-local: $(MK_CA_DOCS) to build it by default.

  • Before 8.6.0, curl.1 was distributed since it was present in noinst_man_MANS, which was added to EXTRA_DIST.

    • As above, eefcc1b dropped noinst_man_MANS from EXTRA_DIST, which caused curl.1 to no longer be distributed.
    • cf5f604 added it back through a dist-hook copying it to curl.1.dist. This is fine, but this could have also been done by adding curl.1 to EXTRA_DIST, which is what was done prior to 8.6.0 (indirectly). If you do end up sticking with the dist-hook and naming it curl.1.dist, there is some other logic that can be removed:

      curl/docs/Makefile.am

      Lines 115 to 127 in 922091c

      # $(abs_builddir) is to disable VPATH when searching for this file, which
      # would otherwise find the copy in $(srcdir) which breaks the $(HUGE)
      # rule in src/Makefile.am in out-of-tree builds that references the file in the
      # build directory.
      #
      # First, seed the used copy of curl.1 with the prebuilt copy (in an out-of-tree
      # build), then run make recursively to rebuild it only if its dependencies
      # have changed.
      $(abs_builddir)/curl.1:
      if test "$(top_builddir)x" != "$(top_srcdir)x" -a -e "$(srcdir)/curl.1"; then \
      $(INSTALL_DATA) "$(srcdir)/curl.1" $@ \
      && touch -r "$(srcdir)/curl.1" $@; fi
      cd cmdline-opts && $(MAKE)

      curl/docs/Makefile.am

      Lines 34 to 36 in 922091c

      # EXTRA_DIST breaks with $(abs_builddir) so build it using this variable
      # but distribute it (using the relative file name) in the next variable
      man_MANS = $(abs_builddir)/curl.1

      I believe this was only needed to make the distinction between the distributed curl.1, and the built curl.1 for out-of-tree builds. If the distributed file is called curl.1.dist, I think this tricky logic can be removed. However, for uniformity, if one distributed manual using the .dist naming scheme, so should the others (curl-config.1 and mk-ca-bundle.1).

As described above, some automake variables set by curl aren't actually used by automake. I think noinst_man_MANS and nodist_MANS should be removed from docs/Makefile.am, docs/libcurl/Makefile.am, and docs/libcurl/opts/Makefile.am, since it just adds to the confusion.

I can work on a pull request to resolve these issues, but first, maybe you could confirm the desired behavior? I think it is

Man page Built by default target Installed Distributed
curl.1 yes yes yes
curl-config.1 yes yes yes
mk-ca-bundle.1 yes no yes
docs/libcurl/*.3 yes yes ?
docs/libcurl/opts/*.3 yes yes ?

Additionally, should distributed man pages be named foo.[1-9] or foo.[1-9].dist?

I expected the following

No response

curl/libcurl version

Current master 5a4b2f9

operating system

N/A

@bagder
Copy link
Member

bagder commented Feb 10, 2024

So what is the issue?

Additionally, should distributed man pages be named foo.[1-9] or foo.[1-9].dist?

This seems to imply you have not understood how dists are created: all .dist files are included in the distribution but with the .dist extension removed. I propose you check how the tarball actually looks like after you run maketgz.

Or check how current dists look like as daily snapshots.

@michaelforney
Copy link
Contributor Author

So what is the issue?

  1. mk-ca-bundle.1 is getting installed again. I presume that isn't intended since it wasn't before 8.6.0. This changed in eefcc1b, and then a revert of that behavior was done in a911f4f, but then it was reintroduced again in 755b31d. If it's intended for it to be installed, then no change is necessary, but since a911f4f was merged, I assume it wasn't intentional.
  2. The docs Makefiles set several bogus automake variables (noinst_man_MANS, nodist_MANS) which seem like they do something, but don't, and as far as I can tell, were never supported by automake.

Additionally, should distributed man pages be named foo.[1-9] or foo.[1-9].dist?

This seems to imply you have not understood how dists are created: all .dist files are included in the distribution but with the .dist extension removed. I propose you check how the tarball actually looks like after you run maketgz.

Indeed, my apologies. I wasn't familiar with how dist-hook worked and just made some assumptions. I retract my comments about the distributed file name.

Or check how current dists look like as daily snapshots.

Thanks for the link.

@michaelforney
Copy link
Contributor Author

One last issue I'd lilke to bring up: although curl.1 is distributed once again, on systems without perl, --disable-docs gets auto-selected, preventing its installation (though, I suppose it could be done manually by the packager).

Should --disable-docs disable installation in both docs and docs/libcurl? Only the latter actually requires perl, since those without it can just use the distributed copies. Maybe it could be renamed/repurposed as --disable-libcurl-docs, analogous to the cmake option BUILD_LIBCURL_DOCS?

@bagder
Copy link
Member

bagder commented Feb 10, 2024

My thinking has been:

  • don't include all the generated man page in the tarball since they can be generated and the .md versions of the files are still quite readable. There are some .1 files still included, but I didn't care much about those.
  • the curl.1 is still shipped generated because of legacy and because it is used in the build of the tool, which then can still be done without the need of perl nor nroff
  • --disable-docs skips generating (and installing) any nroff docs, except the ones that are shipped in the tarball: this should make the build work without requiring perl

@vszakats
Copy link
Member

vszakats commented Feb 10, 2024

Distributed curl.1 is also required for reproducible builds (due to nroff output being env-dependent, and tool_hugehelp.c depending on curl.1).

@bagder
Copy link
Member

bagder commented Feb 18, 2024

@michaelforney is there any still outstanding issue in the current tarballs?

@michaelforney
Copy link
Contributor Author

Yes, it doesn't look like anything has changed since I opened this issue.

To reiterate:

  1. mk-ca-bundle.1 is installed with make install:
$ tar xJf curl-8.6.1-20240219.tar.xz
$ cd curl-8.6.1-20240219
$ ./configure --with-openssl --disable-manual
<snip>
$ make install DESTDIR="$PWD/stage"
<snip>
$ find stage -name '*.1'
stage/usr/local/share/man/man1/curl.1
stage/usr/local/share/man/man1/mk-ca-bundle.1
stage/usr/local/share/man/man1/curl-config.1
$
  1. Distributed man pages curl.1 and curl-config.1 are not installed on systems without perl.
$ perl
/bin/ksh: perl: not found
$ tar xJf curl-8.6.1-20240219.tar.xz
$ cd curl-8.6.1-20240219
$ ./configure --with-openssl --disable-manual
<snip>
$ make install DESTDIR="$PWD/stage"
<snip>
$ find stage -name '*.1'
$
  1. docs/Makefile.am, docs/libcurl/Makefile.am, and docs/libcurl/opts/Makefile.am set bogus automake variables nodist_MANS and noinst_man_MANS. This has no actual adverse affect but it is confusing and misdirecting for those trying to understand the docs Makefile.am files.

bagder added a commit that referenced this issue Feb 19, 2024
Fixes #12921
Reported-by: Michael Forney
bagder added a commit that referenced this issue Feb 19, 2024
Fixes #12921
Reported-by: Michael Forney
bagder added a commit that referenced this issue Feb 19, 2024
Drop docs/mk-ca-bundle.1 from the tarball. It can be generated at will.

Fixes #12921
Reported-by: Michael Forney
@bagder bagder closed this as completed in ab027d9 Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants