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

New configure option to disable building and installing library manpages #8835

Merged
merged 2 commits into from Oct 5, 2019

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jul 26, 2019

This PR adds ---disable-stdlib-manpages to configure to disable the compilation of the standard library and compilerlibs man-pages

@Octachron
Copy link
Member

Personally, I agree with the idea of moving the generation of man pages to the test part of the build process.

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

Thanks, @Octachron! Note it's only CI which is testing it - I guess we could make part of running the testsuite also test that the docs build when run locally? Personally I think it's OK to catch errors in documentation comments in CI - hopefully if someone is actually writing documentation comments then they're also reading them while developing!

@alainfrisch
Copy link
Contributor

Why do we build manpages for the stdlib? Is anyone using them?

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

I've put this in the Working version section of Changes, but I'd propose changing it for 4.09 as well, if that's OK?

I intend to backport it to all the ocaml-base-compiler packages in opam-repository, so it's sort of moot which version we commit it in!

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

@alainfrisch - I don't know why we were ever building them... as far as I can see they've never been part of make install. Worth building once in CI just as it checks the man backend for ocamldoc.

@dbuenzli
Copy link
Contributor

Note that system packagers seem to install them (but I personally don't think man is such a great medium for API docs).

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

Oops - I've re-checked the Makefile having looked at Debian's package more closely. We do install the man pages if they've been built.

So, I'd like to propose we stop doing that by default, but I'll push a configure option to enable it instead.

@gasche
Copy link
Member

gasche commented Jul 26, 2019

I personally use manpages for the stdlib module very often. It's the quickest way for me to answer the question of "what's the API of Stream again?" when I'm not in my editor (there Merlin does it, but it's harder to get the docs than just the types).

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

I should have checked ocamldoc/Makefile's install target more closely - so this is a breaking change. However, I'd still like to propose making it, even if putting it into 4.09 at this stage may be too late.

@gasche
Copy link
Member

gasche commented Jul 26, 2019

If the manpages are currently built and installed, I don't think that we should remove them. Having a configure option to not build them seems fine, but I think it should not be the default. @dra27, are you sure that the PR is worth it in this context?

(There may be other ways to reduce the ocamldoc-related build time, such as building the manpages using native ocamldoc rather than bytecode ocamldoc when it is available.)

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

On the high performance server I was testing this on, building those man pages takes 12 seconds. I’d be amazed if switching to ocamldoc.opt will get it below the 1 second which I’d regard as tolerable! For opam-repository, I’ll propose a mechanism whereby you can keep building them, but it seems a vast amount of cpu-time across all users to devote to something which is probably not used that much.

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

It might be informative to have a poll on this comment with:

  • 👍 meaning one uses the stdlib manpages,
  • 👎 meaning you know they exist and never use them, and
  • 😕 meaning you didn’t know so much of your CPU time was spent building something you didn’t even know was there! 🙂

@gasche
Copy link
Member

gasche commented Jul 26, 2019

Of the six people that chimed in so far, at least one (myself) uses the feature on a regular basis. I don't think anyone knows how often this feature is used.

I'm sure there are many ways to save CPU time off the compiler build that do not impact the observable result for users. For example, several things in the standard distribution are built using ocaml{c,opt} instead of ocaml{c,opt}.opt -- ocamldoc is an example, ocamltest is another (why is ocamltest built by default anyway?), the debugger, etc. I would warmly recommend that we explore those options first.

@gasche
Copy link
Member

gasche commented Jul 26, 2019

Note: on my machine using ocamldoc.opt goes from 15s to 5s.

@gasche
Copy link
Member

gasche commented Jul 26, 2019

Of the 5s spent building the manpages, around 4s are spent "anlayzing" the input files (which include both the stdlib and compiler-libs, that is basically everything), and around 1.6s are spent producing the manpages. Analyzing the input files is work that could be shared between all the ocamldoc backends by using the -dump and -load options of ocamldoc (which can (de)serialize the analysis results). Building the manpages from a dump takes 1.8s on my machine.

@dbuenzli
Copy link
Contributor

@dra27 I'm not sure a poll on the people watching this repo will be representative of the actual users of these manpages.

@gasche I don't know what the plans for ocamldoc are but if it is eventually ditched in favor of odoc I'm not sure the days of these manpages are not counted anyways. But I discovered a few days ago that odoc's HTML pages are pleasant to read into a text mode browser (namely eww). For now odig doc only knows how to open package pages which is not exactly your usage but I hope it can be made to open arbitrary identifiers of your install base, see this issue. This might provide you with a nice terminal reading experience with hyperlinks, by setting your BROWSER variable to an appropriate value.

@cfcs
Copy link

cfcs commented Jul 26, 2019

  1. FWIW the manpages are installed on debian by default, and also by opam switches (opam initializes the MANPATH environment variable to make your current switch take precedence).

  2. I personally use ocp-browser for the use case @gasche described. I've found it useful from time to time when ocp-browser wasn't building/working for whatever reason, but in any case I agree with @dbuenzli that man seems inferior to other documentation formats (HTML, GNU info, and other hypertext variants where you have proper hyperlinks), and spending time building them seems a bit of a waste of energy. The downside of ocp-browser is of course that it requires installation, and I don't know how to scroll to read long docstrings :)

  3. The generated manpages do not reference the package required to use the module. Having a manpage for (some) Targetint module is nice, but I still have to go out of my way to figure out how to use it, if it's even exposed. And there's things like the names of modules like String and Unix kind of clashing with pre-existing man 3 string (glibc string functions) and man 7 unix (unix sockets) manpages on my system.

  4. When used in CI systems and environments like the one @dra27 describes it seems kind of annoying to have to generate manpages that will never be looked at. That being said, adding more parametric/optional compilation modes means exploding the build matrix for OCaml itself, so I would be in favor of either keeping the current behavior or removing it altogether.

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

@dbuenzli - do you honestly think I wasn’t planning on advertising the question more, if the poll were agreed to be of use?? 🙂

@gasche - the optimisations are interesting, although that still doesn’t save any time in a present opam switch, given that only the man pages are built. Can the analysis ever be parallelised, though? The other optimisations are, at least in the current build system setup, very invasive (the TL;DR is that you’re trying to use tools built during the “opt.opt” stage during its predecessor “opt” stage... that’s not to say I don’t think it’s a good idea)

@dra27
Copy link
Member Author

dra27 commented Jul 26, 2019

I’d be happy to make the change here less invasive by keeping the default to build the manpages, but push for a downstream change of configure options in opam-repository?

@Octachron
Copy link
Member

Personally I think that a good part of the problem is that the man pages are built far too soon. When iterating on the compiler, I would prefer if they were built just before the install target or during the tests themselves (as a test for ocamldoc) and not as a part of the world target.

I was also wondering if it could make sense to have an ocaml-stdlib-manpages package on opam that would mirror the ocaml-manual package.

@cfcs Recent versions of the compiler-libs module documentation mention in their introduction that the modules are part of compiler-libs.

@dbuenzli , as far as I am concerned, the lack of non-html backend in odoc is one of the reason why we are still using ocamldoc rather than odoc for the manual.

@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 26, 2019

@dbuenzli , as far as I am concerned, the lack of non-html backend in odoc is one of the reason why we are still using ocamldoc rather than odoc for the manual.

TBH I'm not sure odoc will get any other backend any time soon -- nor that it is actually desirable, there's already so much to do and so little progress there.

As much as I like manuals and PDFs, I'm not sure I see the point of having the Stdlib API in the PDF manual; would I print the latter that's the first thing I would elect to trim out... But if you really want that including the .mlis via lstlisting works equally well in my opinion; with a nice retro-style touch.

Then there is also the texinfo backend but then if you are an OCaml programmer you are unlikely to use only the Stdlib and as far as I know this is the only API you will find in this documentation format so I doubt you would favor that mean of access. Regarding the texinfo manual itself it has been broken for users for more than 15 years. As I mentioned above if you are really averse to browsers (which I can understand) I think a text mode browser provides an experience on the API doc and manual similar to what info would (if you are an emacs user try M-x eww and browse here).

I think the (sadly) little manpower and people OCaml has to work on documentation infrastructure could be better used than trying to maintain ocamldoc indefinitely for these usages.

@gasche
Copy link
Member

gasche commented Jul 27, 2019

I'm moving to close this PR. (It's a bit of a forceful move, but then I'm a bit scared by the amount of energy that is spent discussing next-century documentation formats when we all have many other cool things to be doing.)

Initially @dra27 had spotted some computations during the build that had no observable effects for users, and decided to remove them to save time. That was a very good idea! It turns out that his initial assessment was wrong, it's a feature visible by users and they do use it. But still there is the suggestion to remove it, here on in the opam-repository configuration. That is a very bad idea!

  • We don't remove features unless there is strong justification for that. (Maintenance burden, security, etc.).
  • The justification given here is to save 15s of CPU time, but this is not strong at all. As I hinted above, there are many other ways to save build time that do not impact users, so go explore them first. (A quick investigation suggested that a simple one, using ocamldoc.opt, already saves 10s.)

Since when has "reducing build time" become such a priority that it justifies removing features? A few weeks ago we finally merged (with @dra27's help) the switch to make -j in the opam-repo; this saves 2m20s of build time (not cpu time) on my machine, and we could have done that for months, and the reason it hadn't been done before is that nobody cared enough to do the opam-repository work.

Leave my manpages in peace!

@gasche gasche closed this Jul 27, 2019
@gasche
Copy link
Member

gasche commented Jul 27, 2019

One minor side-effect of my irritation in a train to the Alps yesterday was #8836, so there is that.

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

Is there any chance you could answer my question about inverting the default, rather than somewhat dictatorially closing the entire PR?

The CI change is an improvement; the addition of an entirely safe configure option (which doesn’t expand the CI test matrix) is an improvement. No features are or have ever been removed in this PR - it was about changing what was built by default

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

@Octachron - indeed, having a package using a similar technique as graphics (which can be built after the compiler in opam-repository) was what I had in mind.

@gasche
Copy link
Member

gasche commented Jul 27, 2019

Sure, building the other backends as well in the CI may be an improvement (if we now want to spend more CPU time with the documentation generation); please feel free to open another PR with this two-line change.

I'm not sure what your question is on inverting the default. If you suggest that we stop building and installing manpages (by default), then I see this as a regression. The only justification brought up so far is the build time, and on that basis I don't think we should do it.

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

I’ve added a configure --enable-stdlib-manpages, the existence of which is clearly useful and and was already suggested by Xavier in May. My suggestion is to change it to --disable-stdlib-manpages, which presents no regression.

Can you dial down your grumpiness? There’s a clear distinction between testing in CI and every single opam user’s build! I get made grumpy having no option to disable building something which on my platform doesn’t have a reader...

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

I'm not sure what your question is on inverting the default.

Right, so if you don’t understand my question, could we have an “ask for clarification, then close PRs” approach?

@dbuenzli
Copy link
Contributor

FWIW I agree with @gasche here. I think it's already painful to have a separate ocaml-manual package (especially since that one usually does not exist for the latest version until I do it :-) and I don't understand why such circumvolutions are needed. @dra27 what is wrong with simply adding --disable-stdlib-manpages { os = "win32" } to the ocaml package file ?

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

@gasche - the discussion for what may or may not happen on opam-repository is for another PR, and not on this repo. The rationale is that building the manpages is clearly not a necessary part of the build, and so it should be possible to disable it. No level of optimisation of the building negates that rationale, even ignoring your wholly incorrect assertion that build system "tweaks" would be simpler than this configure-driven removal of a single dependency from a single target.

@hcarty
Copy link
Member

hcarty commented Jul 27, 2019

Saving 15 seconds every time a new opam switch is created is a non-trivial speedup. Even more so on Windows where we may be paying extra system abstraction costs through things like cygwin.

Making the man pages a separate opam package seems like a very reasonable compromise. I know some people will want the man pages on every install. Some people (me!) will also want the OCaml manual + stdlib docs rendered with odig on most switches, along with installations of dune, merlin and other development niceties. That doesn't seem like a good reason to impose those on every user or every environment.

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

@dbuenzli - that's what I was suggesting? The extra package, if done that way, permits one to have a mechanism to override that setting. At this stage, the discussions about what gets done on opam-repository seem rather off the topic of this PR.

@gasche
Copy link
Member

gasche commented Jul 27, 2019

I'm personally not satisfied with these explanations. I still don't see the point of all that work, and I think it's a mistake to keep dragging all of us in it. (The initial time measurement motivated me to go look at how to speed things up -- with no observable difference for users whatsoever -- and I would warmly encourage you to do this as well.)

@dra27
Copy link
Member Author

dra27 commented Jul 27, 2019

Sorry, by "that work" - you mean the addition of a configure-option in this PR or any future changes in opam-repository?

@gasche
Copy link
Member

gasche commented Jul 27, 2019

In an on itself, I think that having a configure option to disable manpage generation sounds reasonable (and I would trust you to do it right on a technical level). What I find concerning is that after I made very clear that I was opposed to the idea of not installing manpages (I don't feel very strongly about this, but it's also a clearly stated opinion), and that your justifications for doing so were unsatisfying (I explained why), you are now (1) proposing (for example in #8835 (comment)) to keep the default unchanged in the compiler distribution but change the default in opam-repository (that is: to stop installing mapages to users by default), while at the same time (2) suggesting that the opam-repository part is out of scope for discussion (whereas it seems to be your whole reason to have this option available in the first place), and (3) avoiding to provide any additional justification that I can understand.

What I see of the whole thing is that it's unjustified change (or at least: change with no good justification provided) that doesn't give us much benefits, and takes us time to discuss, decide and implement -- here and, yes, on the opam-repository level as well, if that is your plan. I don't understand why we haven't dropped this PR yet.

@gasche
Copy link
Member

gasche commented Jul 27, 2019

In #8837 I propose to use ocamldoc.opt to build the manpages, bringing the build time from 14s to 4s on my machine.

@gasche
Copy link
Member

gasche commented Jul 28, 2019

In #8839 I propose to use ocaml{c,opt}.opt when available to build ocamldoc (to be generalized to other tools of course), cutting the sequential build time of the ocamldoc binaries from 31s to 11s on my machine (from 16s to 6s in parallel).

@xavierleroy
Copy link
Contributor

That's a heated discussion! I didn't know ocamldoc could generate so much passion and effort :-)

In the medium to long term, ocamldoc could move out of the core distribution and live as a standalone package, obviating this discussion.

Meanwhile, I agree with @dra27 a configure option to control man page generation could be useful in a CI setting, but I agree with @gasche that the default should be to generate man pages, because some users and packagers rely on this.

@dra27
Copy link
Member Author

dra27 commented Jul 30, 2019

Thanks, @xavierleroy - just to confirm that the present form of the PR does indeed preserve the default of building the manpages. Are you happy to approve this (and is @gasche happy to lift the veto?)

Gabriel and I moved our discussion offline - we have different opinions on the relationship between OCaml and opam-repository, but both from that discussion and comments here, I don't intend proposing any changes in opam-repository to take advantage of this without some downstream changes in the opam client.

@shindere
Copy link
Contributor

One implementation-related comment. I'm not very found of the 'manpages'
and '' values for the 'stdlib_manpages' variable. I know other
varialbes already work that way, but that's the older ones. The newer
variables use 'true' and 'false'. I do realise it will make the
code in the Makefile a bit more involved but still find the change
suitable. Would you accept to do that change @dra27?
Or, if I submit a patch to you PR, would that be fine with you?

@dra27
Copy link
Member Author

dra27 commented Sep 29, 2019

Rebased and updated to address @shindere's request

@gasche
Copy link
Member

gasche commented Sep 29, 2019

I forgot to say it explicitly earlier, but I'm okay with adding a configuration option to disable building and installation of manpages. (It would make sense to me that manpages be disabled by default on Windows, whose users don't use manpages at all.)

@dra27
Copy link
Member Author

dra27 commented Sep 30, 2019

Thanks, @gasche! I'm happy to leave the selection of the default to "elsewhere" (i.e. allow opam or whatever to utilise the option to disable the building of them on Windows) and keep the logic simpler in configure (for my own development, #8995 allows me to have my preference on local builds).

@dra27 dra27 closed this Sep 30, 2019
@dra27 dra27 reopened this Sep 30, 2019
@dra27
Copy link
Member Author

dra27 commented Oct 5, 2019

Is this change now approved?

@gasche gasche merged commit e232369 into ocaml:trunk Oct 5, 2019
@dra27 dra27 deleted the save-the-planet branch October 5, 2019 16:41
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.

None yet

10 participants