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

Spring cleaning of the manual's build process #1781

Merged
merged 8 commits into from May 20, 2018

Conversation

Projects
None yet
6 participants
@steinuil
Copy link
Contributor

commented May 10, 2018

This patch cleans up the build process for the manual and its tools. I took care not to break anything or change the outputs of the build (other than renaming a few intermediate files).

  • Unused tools were removed, along with old .*ignore files coming from other VCS.
  • make clean should now delete all generated files.
  • make pregen and pregen-etex now only rebuild files as needed rather than invoking ocamldoc every time. The tools are now also only rebuilt as needed, so calling make pregen with no changes from the last invocation takes 0.10s rather than ~3.70s.
  • Rebuilding just the html manual with no changes to the stdlib reference now takes about 2.5s rather than 7.7s.

I also tried to improve the code quality in the Makefiles a bit.

The manuals are still fully rebuilt with each invocation even if there were no changes, but fixing that would take a bigger rewrite, and it wouldn't make the builds any faster when a single file has changed anyway. Instead I tried to optimize for building the html manual faster, so that iterative changes to the docs will be somewhat quick if you build with make html.

@pmetzger

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Thank you so much for working on this, @steinuil! I haven't had a chance to look at all the patches yet, but it certainly looks like it was a significant amount of effort.

@mseri

mseri approved these changes May 18, 2018

Copy link
Member

left a comment

Wow this is a nice cleanup, in addition of getting rid of some unnecessary rebuilds, the resulting code is more readable and the syntax more uniform. There are still a lot of loops to go through to follow what is going on but I think it is a good step forward.

If I am not mistaken the only thing missing to have a green CI tick, is update the ChangeLog file. I'm not the best person to suggest which section but I believe ### Compiler distribution build system could be it

tools:
cd tools; ${MAKE} clean; ${MAKE} all
$(MAKE) -C tools all

This comment has been minimized.

Copy link
@mseri

mseri May 18, 2018

Member

Why did you decide to drop the make clean step here, just to reduce further the rebuilds?

This comment has been minimized.

Copy link
@steinuil

steinuil May 18, 2018

Author Contributor

Yes, I'm not really sure why the tools used to get rebuilt each time because I'm pretty sure they don't depend on anything that changes with each rebuild of the manual.

This comment has been minimized.

Copy link
@gasche

gasche May 18, 2018

Member

The make clean makes sense from the perspective of someone who changes the compiler codebase often, and builds the manual rarely. What's at risk of changing is not the manual build artefacts, but the compiler itself (which is used to build the tools). If the manual/tools Makefile hasn't properly specified its dependencies on the compiler tools (which is hard to do: compilers, standard library, etc.), then it is safer to just rebuild them each time we rebuild the manual, to guarantee that they have been built with the latest compiler version. Otherwise you could get into weird issues when, for example, we change the executable magic numbers, where the tools are compiled with an older compiler version and they refuse to run with the new OCaml runtime during the manual build.

(Then it makes sense to have this clean in this manual/ Makefile rather than in the all: rule in manual/tools, as the latter is likely to be used by people who quickly iterate changing the tools, while the former is used by everyone trying to build the manual and not familiar with the subtleties of the build system.)

To summarize: there could be a better way to do this, but it sort of makes sense to me that the clean rule is there.

This comment has been minimized.

Copy link
@steinuil

steinuil May 18, 2018

Author Contributor

Right, I didn't consider that. It shouldn't add much to the build times anyway, since there's not many tools to build anymore.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 18, 2018

Member

Just an aside: while fixing the manual, I often re-build it many times in an hour, so getting the dependencies right eventually would be best.


.PHONY: check-cross-references
check-cross-references: cross-reference-checker
$(OCAMLRUN) ./cross-reference-checker \

This comment has been minimized.

Copy link
@gasche

gasche May 18, 2018

Member

While we are at it, this $(OCAMLRUN) invocation should probably be $(SET_LD_PATH) $(OCAMLRUN) as the others; see 482e8a8.

@gasche gasche requested a review from Octachron May 18, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented May 18, 2018

I wonder about removing dead tools. It is a reasonable idea, but it may be possible that their code would be useful for people interesting in writing new tools. @Octachron, you are the manual-tools expert; do you think the tool codebases removed in this PR had any value?

@steinuil

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

I asked Octachron a few days ago on the IRC and he said it was probably a good idea, and most of the old tools are just old versions of the current ones from what I could tell.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented May 18, 2018

My opinion is indeed that the dead tools should be removed or transferred to somewhere else. More precisely, amongst those tools, we have

  • caml-tex: a perl precursor of caml_tex2
  • format-intf: an ocamldoc precursor
  • htmlquote.c: a variant/precursor of texquote2.c that was already not used at the time of the migration from Caml Special Light
  • dvi2text, texexpand, latex*, htmlthread, htmltbl, , htmlcut : some hevea precursor tools in perl or OCaml which are not used since f44aa99 .

Consequently, I don't think that those tools would be really useful to anyone trying to write new tools for the OCaml manual.

@steinuil

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

The last commit should have fixed all those problems. Should I squash some commits together or is it fine like this?

@pmetzger

This comment has been minimized.

Copy link
Member

commented May 18, 2018

@steinuil My taste is always to squash commits so the main history of a project remains maximally readable.

@steinuil steinuil force-pushed the steinuil:html-manual branch 4 times, most recently from e6fe562 to c39e5b0 May 18, 2018

@steinuil

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

@pmetzger that's what I would do, but I made a lot of changes in this PR so I thought that squashing it all together would decrease readability rather than aid it.

Anyway, I squashed together what I thought it'd make sense to have together. If it's all good I'll add an entry to the changelog. Is the ### Compiler distribution build system section ok?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented May 18, 2018

The build system section of Changes.md is the right section.

@pmetzger

This comment has been minimized.

Copy link
Member

commented May 18, 2018

Looks like there are conflicts now...

@Octachron
Copy link
Contributor

left a comment

LGTM: I like the changes, they improve the readability of the Makefiles, and fix few dependencies issues. I only have few minor remarks or self notes on dead code.


dvi2txt:
cd dvi_to_txt; ${MAKE}

transf: transf.cmo htmltransf.cmo transfmain.cmo

This comment has been minimized.

Copy link
@Octachron

Octachron May 19, 2018

Contributor

Note for later: htmltransf is not used, it is only used if transf is called with the -html option, which is not the case anymore.

cd texstuff \
&& sh ../../tools/fix_index.sh pdfmanual.idx \
&& makeindex pdfmanual.idx \
&& makeindex pdfmanual.kwd.idx

This comment has been minimized.

Copy link
@Octachron

Octachron May 19, 2018

Contributor

Would it be possible to avoid the duplication of the rules of index:: … ?

manual.hva -e macros.tex ../manual.tex \
&& $(HACHA) -tocter manual.html

htmlman/libref/style.css: style.css $(STDLIB_MLIS) | unprefix_stdlib_for_ocamldoc

This comment has been minimized.

Copy link
@Octachron

Octachron May 19, 2018

Contributor

I think it would be nice to mention in a comment that htmlman/libref/style.css is used as witness for the generation of the html doc by ocamldoc.

LD_PATH := "$(SRC)/otherlibs/unix/:$(SRC)/otherlibs/str/"
SET_LD_PATH = CAML_LD_LIBRARY_PATH=$(LD_PATH)

VPATH = ".:$(STDLIB_DIR):$(CSLDIR)/parsing:$(CSLDIR)/otherlibs/unix:$(CSLDIR)/otherlibs/str:$(CSLDIR)/otherlibs/graph:$(CSLDIR)/otherlibs/threads:$(CSLDIR)/otherlibs/dynlink:$(CSLDIR)/otherlibs/bigarray"

This comment has been minimized.

Copy link
@Octachron

Octachron May 19, 2018

Contributor

Note: this VPATH is no longer used due to the acrobatic way the unprefixed stdlib mlis and cmis are currently produced.

@steinuil steinuil force-pushed the steinuil:html-manual branch 2 times, most recently from cf7f4de to f616ac1 May 19, 2018

@steinuil steinuil force-pushed the steinuil:html-manual branch from f616ac1 to 16f8372 May 19, 2018

@steinuil

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2018

Thanks for the review, I made the changes and squashed them into the previous commits.

@pmetzger

This comment has been minimized.

Copy link
Member

commented May 19, 2018

@steinuil There are some conflicts, FYI. You need to fix them so that the merge can happen.

@gasche gasche merged commit 385ac89 into ocaml:trunk May 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented May 20, 2018

Merged. Thanks everyone for your work and feedback.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

My opinion is indeed that the dead tools should be removed or transferred to somewhere else.

As the author of most of those tools, I agree. For the most part they were made obsolete by Hevea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.