Merge ocamldoc/Makefile into the root Makefile#12616
Merged
dra27 merged 8 commits intoocaml:trunkfrom Oct 20, 2023
Merged
Conversation
dra27
reviewed
Oct 14, 2023
Member
dra27
left a comment
There was a problem hiding this comment.
As ever, keeping up the more lines deleted than added (just!) :D
Mostly hair-spitting - odoc_test.ml should either be being removed (unlikely) or continue to be built. I think it's worth going slightly further with the refactoring of the WITH_* variables, but that's not compulsory.
We're very nearly there!
2bc27b4 to
d1710fe
Compare
Contributor
Author
|
Thanks for the review!
David Allsopp (2023/10/14 06:11 -0700):
Mostly hair-spitting - odoc_test.ml should either be being removed
(unlikely) or continue to be built.
My bad. Fixed. The interesting thing to note here is that this module is
used by the testsuite but that each test that uses it rebuilds it. And
building it during ocamldoc's build won't save the five builds during
the tests (which is why the omission remained unnoticed), but I
agree it's still good to build odoc_test while ocamldoc is
being built, because that's what is done so far and to make sure the
module compiles without having to run the testsutie to verify this property.
In Makefile:
> + $(MAKE) otherlibraries $(WITH_DEBUGGER) $(OCAMLDOC_TARGET) \
$(WITH_OCAMLTEST)
A possible thought for this - rather than having an individual variable
for each of these targets, could we instead have $(OPTIONAL_TOOLS)
which gets set-up with the correct options from the debugger, ocamltest
and ocamldoc?
I think it's possible, yes. As discussed offline, I propose to leave
that, and a few others, for a cleanup PR that will be easier to do
separately, once all the merges are finished.
I added a comment to keep track of the idea, though.
In Makefile:
> @@ -2425,7 +2425,7 @@ endif
ifeq "$(build_ocamldoc)" "true"
$(MAKE) -C ocamldoc install
endif
-ifeq "$(WITH_OCAMLDOC)-$(STDLIB_MANPAGES)" "ocamldoc-true"
+ifeq "$(build_ocamldoc)-$(STDLIB_MANPAGES)" "true-true"
Given that it's changing before merging, I have ***@***.*** from a
while back which tried to simplify this further - a version of that
might be worth pulling in now? I can't remember the exact reason it
ended up this way with two variables controlling this decision, but
everything is known at configure-time
That's a good idea, thanks!
I cherry-picked and adapted your commit and it's split into two parts:
the renaming of the variable from STDLIB_MANPAGES to
build_libraries_manpages (the former being kept public for backward
compatibility, the latter being defined privatel), and a seocnd commit
that makes thevalue of `build_libraries_manpages` more accurate, taking
into account wether ocamldoc has been configured or not.
Worthnoting is that it's not only the manpages for the standard library
that we are building, but also those for the compilerlibs and the
otherlibs. Hence the use of libraries_manpages rather than
stdlib_manpages. To be completely coherent it would make sense to also
rename the corresponding configure option from --enable-stdlib-manpages
to e.e. --enable-libraries-manpages (but that would break code) or to
provide the new option as an equivalent ot the old one (but that would
yield code dumplicaiton). Since none of these two solutions looked
appealing, I stayed with what we currently have but can change it if
people so wish.
In Makefile:
> +ifeq "$(build_ocamldoc)" "true"
+OCAML_LIBRARIES += ocamldoc/odoc_info
+endif
In the same idea as having a list of optional programs for the all and
opt.opt targets, could we have OTHER_LIBRARIES being set-up in
configure and then included above in OCAML_LIBRARIES directly? (it
would remove the need for the ifeq entirely)
Sure! The comment mentionned above does include this suggesiton as well.
In Makefile:
> @@ -2423,7 +2510,21 @@ endif
$(MAKE) -C otherlibs/$$i install || exit $$?; \
done
ifeq "$(build_ocamldoc)" "true"
- $(MAKE) -C ocamldoc install
+ $(MKDIR) "$(INSTALL_BINDIR)"
This line is no longer necessary (since you can't invoke just the
installation of ocamldoc on its own anymore)
Yes, indeed, thanks. Removed.
In Makefile:
> @@ -2512,7 +2613,21 @@ endif
$(ocamlopt_CMO_FILES) \
"$(INSTALL_COMPLIBDIR)"
ifeq "$(build_ocamldoc)" "true"
- $(MAKE) -C ocamldoc installopt
+ $(MKDIR) "$(INSTALL_BINDIR)"
Similarly (although installopt only implicitly assumes that install has
already been called)
Line remove, too. Thanks.
In ocamldoc/Makefile:
> -
-programs := ocamldoc ocamldoc.opt
-
-OCAMLDOC_LIBCMA=odoc_info.cma
-OCAMLDOC_LIBCMI=odoc_info.cmi
-OCAMLDOC_LIBCMXA=odoc_info.cmxa
-OCAMLDOC_LIBA=odoc_info.$(A)
-
-OCAMLDOC_LIBMLIS=$(addsuffix .mli,\
- odoc_dep odoc_dot odoc_extension odoc_html odoc_info odoc_latex \
- odoc_latex_style odoc_man odoc_messages odoc_ocamlhtml odoc_parameter \
- odoc_texi odoc_text_lexer odoc_to_text odoc_type odoc_value)
-OCAMLDOC_LIBCMIS=$(OCAMLDOC_LIBMLIS:.mli=.cmi)
-OCAMLDOC_LIBCMTS=$(OCAMLDOC_LIBMLIS:.mli=.cmt) $(OCAMLDOC_LIBMLIS:.mli=.cmti)
-
-ODOC_TEST=odoc_test.cmo
This doesn't appear to have been merged? If I read correctly, the file
was always built before?
It was yes and I added it again explicitly to the ocamldoc target.
Thanks.
|
dra27
reviewed
Oct 19, 2023
Member
dra27
left a comment
There was a problem hiding this comment.
A minor suggestion in configure.ac, but otherwise this is looking good to go!
configure.ac
Outdated
Comment on lines
+2257
to
+2258
| [AS_IF([test x"$enable_stdlib_manpages" = "xyes"], | ||
| [AC_MSG_ERROR([--enable-stdlib-manpages requires ocamldoc])]) |
Member
There was a problem hiding this comment.
I would put this test much higher up, around L532 just after the checks for the dependencies of ocamldoc - the reason is that ./configure --disable-ocamldoc --enable-stdlib-manpages does an awful lot of work before returning the error relative to ./configure --disable-unix-lib --enable-ocamldoc
This is captured by the generic framework and does thus not need to be written here. (Follow-up to PR ocaml#12321 merging ocamltest/Makefile into the root Makefile)
This variable was used in ocamldoc/Makefile, by the test targets that got removed in PR ocaml#12615.
This commit introduces two private build variables: OCAMLDOC_TARGET and OCAMLDOC_OPT_TARGET.
The ./ prefix in front of $(OCAMLDOC) and $(OCAMLDOC_OPT) is not useful since the definitions of these variables are already prefixed with $(ROOTDIR).
Makefile.best_ocamldoc was included in two files: 1. In ocamldoc/Makefile, which also includes $(ROOTDIR)/Makefile.best_binaries So that the definitions which were in Makefile.best_ocamldoc remain available. 2. In api_docgen/ocamldoc/Makefile which also includes Makefile.best_binaries via api_docgen/Makefile.common
STDLIB_MANPAGES is still defined in Makefile.config.in for backwards compatibility, whereas build_libraries_manpages is defined in Maekfile.build_config.in and thus remains private. Co-authored-by: David Allsopp <david.allsopp@metastack.com>
d1710fe to
c69b429
Compare
Before this commit, the build_libraries_manpages variable (aka STDLIB_MANPAGES) was set to true if the build of the manpages for libraries was enabled at configure time, whether ocamldoc (which is required to build those manpages) was enabled or not. It was thus the build system's responsibility to determine whether the manpages for libraries should be built / installed, by testing both the build_ocamldoc and the build_libraries_manpages variables. However, it is known at configure time whether ocamldoc is available or not, which makes it possible to set build_libraries_manpages to true only if the manpages for libraries have been requested AND ocamldoc has been enabled. This is what is done in this commit, leading to a simplification on the build system's side since it becomes enough to test only one variable, namely build_libraries_manpages, rather than two like before. Co-authored-by: David Allsopp <david.allsopp@metastack.com>
c69b429 to
53f2a93
Compare
Contributor
Author
|
David Allsopp (2023/10/19 10:41 -0700):
@dra27 commented on this pull request.
A minor suggestion in `configure.ac`, but otherwise this is looking good to go!
> + [AS_IF([test x"$enable_stdlib_manpages" = "xyes"],
+ [AC_MSG_ERROR([--enable-stdlib-manpages requires ocamldoc])])
I would put this test much higher up, around L532 just after the
checks for the dependencies of ocamldoc - the reason is that
`./configure --disable-ocamldoc --enable-stdlib-manpages` does an
awful lot of work before returning the error relative to `./configure
--disable-unix-lib --enable-ocamldoc`
You are of course right! Thanks! I did notice that
./configure --disable-ocamldoc --enable-stdlib-manpages
took ages before failing but it seems I did not realise there
was somethign I could do about it.
This is now fixed. Thanks again.
|
Member
|
Could we still have an |
Contributor
Author
|
Gabriel Scherer (2023/10/20 06:59 -0700):
Could we still have an `ocamldoc-clean` or `clean-ocamldoc` target to
do what `make -C ocamldoc clean` would do previously? The latter is
useful to me in some cases to get out of an incompatible-digests issue
(by cleaning the two affected directories instead of all
`partialclean`).
Yes, sure, the's no reason why this couln't be done!
My expectation, though, would be that we become able to express
dependencies well enough so that this inconsistent assumptions situation
simply does not occur any longer.
Is the situation you describe reproducible enough for you to be able to
produce a repro case? Ideally, it would be helpful if the repro case
could be on trunk after this has been merged, just in case the merge
would change anything to the situaiton.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR depends on #12586 which has not been merged yet. It start by fixing a left-over from #12321, before doing what is announced.
In addition to the two usual merges (sub-Makefile and sub .depend into the root ones), this PR also merges
ocamldoc/Makefile.best_ocamldocintoMakefile.best_binaries.Three remarks can be made:
.dependandocamldoc/.dependare 8450 and 965 lines long, thus 9415 lines in total, to be compared to the 9464 lines of the root.dependfile on the PR branch. It has been checked that the 49 lines that have been added all correspond to genuine dependencies of ocamldoc either on a library (dynlink, str, unix) or on compiler internals. This PR thus does make the global dependency graph of our source base more explicit and accurate. The fact that we gain more fine-grained dependencies should ultimately allow us to get rid of more gross dependencies and thus increase the parallelism of the build.