Merge testsuite sub-makefiles in the root Makefile and harden build system#12706
Merged
shindere merged 5 commits intoocaml:trunkfrom Nov 30, 2023
Merged
Merge testsuite sub-makefiles in the root Makefile and harden build system#12706shindere merged 5 commits intoocaml:trunkfrom
shindere merged 5 commits intoocaml:trunkfrom
Conversation
08b325c to
25a97c4
Compare
68fe8f7 to
4f5d0de
Compare
dra27
reviewed
Nov 11, 2023
Member
There was a problem hiding this comment.
A few suggestions here and there, with a couple of other thoughts:
- Given the integration into the main Makefile, could we just move expect, codegen, testing and "lib" (which could be given a more descriptive name) to
tools/(in the same way that stripdebug, cmpbyt and other parts of the build system also live in tools)? - I fully support hardening the correct running of the testsuite, but I'm not overly keen on this approach. It is relatively easy to end up working on a tree where ocamltest wasn't built by default, and it's very pedantic and time-consuming to force the user to reconfigure the tree. I would prefer it to be that the test for, say,
diffand everything is always done, but is only an error ifocamltestis explicitly requested and, consequently, that attempting to buildocamltestis only an error if it impossible to build.
|
|
||
| ocamltest: ocamltest/ocamltest$(EXE) \ | ||
| testsuite/lib/lib.cmo testsuite/lib/testing.cma | ||
| testsuite/lib/lib.cmo testsuite/lib/testing.cma testsuite/tools/expect$(EXE) |
Member
There was a problem hiding this comment.
I don't think this needs to be here - isn't testsuite/tools/expect built as part of BYTECODE_OPTIONAL_TOOLS?
|
|
||
| ocamltest.opt: ocamltest/ocamltest.opt$(EXE) testsuite/lib/testing.cmxa | ||
| ocamltest.opt: ocamltest/ocamltest.opt$(EXE) \ | ||
| testsuite/lib/testing.cmxa $(asmgen_OBJECT) testsuite/tools/codegen$(EXE) |
Member
There was a problem hiding this comment.
Similarly, isn't this part of the optional tools variable?
| ocamltest_opt_target=ocamltest.opt | ||
| ocamltest='ocamltest' | ||
| ], | ||
| testsuite_tools='testsuite/tools/codegen testsuite/tools/expect' |
Member
There was a problem hiding this comment.
As a further optimisation (I think) codegen is only needed if the native compiler is being built
Contributor
Author
|
David Allsopp (2023/11/11 06:07 -0800):
* Given the integration into the main Makefile, could we just move
expect, codegen, testing and "lib" (which could be given a more
descriptive name) to tools/ (in the same way that stripdebug,
cmpbyt and other parts of the build system also live in tools)?
This had actually already been discussed in the past, although I can't
remember whether the discussion took place online or off-line. At that
time, the conclusion was that we wanted to keep the tools and libs
related to tests separated from the rest but that decision can of course
always be revisited.
* I fully support hardening the correct running of the testsuite, but
I'm not overly keen on this approach. It is relatively to end up
working on a tree where ocamltest wasn't built by default, and it's
very pedantic and time-consuming to force the user to reconfigure
the tree. I would prefer it to be that the test for, say, diff and
everything is always done, but is only an error if ocamltest is
explicitly requested and, consequently, that attempting to build
ocamltest is only an error if it impossible to build.
David and I discussed this point intensively offline. My understanding
of the conclusion is that, for the time being, the fact that there is an
error hinting the user on what to do is already an improvement over te
previous situation. Ideally, though, in the case of ocamltest (and only
in that case), it seems desirable to make it possible to build it
although it has been disabled at configure time (provided it's actually
possible to do so), so that workflows like `./configure && make -- make
test` work in any case. That means that if ocamltest is enabled then it
will be build during the main build, but if it's not, then it will be
build as part of `make test`. At the moment this is non-trivial to
implement so we think it's best to wait until `testsuite/Makefile` gets
merged into the root Makefile too to implement this feature. I'll let
@dra27 comment on whether this summary of our discussion is accurate.
In testsuite/lib/testing.ml:
>
let failure_test f x s = test_raises_this_failure s f x;;
-let any_failure_test = test_raises_some_failure;;
+(* let any_failure_test = test_raises_some_failure;; *)
Instead of commenting this out, why not expose it in the .mli?
I did so to mark that this bit of code is currently unused.
Given this explanation, what do you prefer? Shall I leave it as it is or
expose it?
In testsuite/lib/lib.mli:
> @@ -0,0 +1,57 @@
+(**************************************************************************)
+(* *)
+(* OCaml *)
+(* *)
+(* Sebastien Hinderer, Tarides *)
FWIW I'd use the copyright header of lib.ml, given that the file is
largely identical
OK, Copyright adjusted
In Makefile:
> @@ -1874,6 +1903,11 @@ partialclean::
rm -rf $(ocamltest_GENERATED_FILES)
rm -f ocamltest/ocamltest.html
rm -f $(addprefix testsuite/lib/*.,cm* o obj a lib)
+ rm -f $(addprefix testsuite/tools/*.,cm* o obj a lib)
+ rm -f testsuite/tools/codegen testsuite/tools/codegen.exe
+ rm -f testsuite/tools/expect testsuite/tools/expect.exe
+ rm -f testsuite/tools/lexcmm.ml # Documentation
The "# Documentation" looks like an editing artefact?
Indeed! Might be the one a few lines down which get copied I don't know
how. Fixed.
In Makefile:
> @@ -1832,7 +1833,7 @@ $(ocamltest_DEP_FILES): $(DEPDIR)/ocamltest/%.$(D): ocam
ltest/%.c
ocamltest/%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS)
ocamltest: ocamltest/ocamltest$(EXE) \
- testsuite/lib/lib.cmo testsuite/lib/testing.cma
+ testsuite/lib/lib.cmo testsuite/lib/testing.cma testsuite/tools/expect$(EXE)
I don't think this needs to be here - isn't testsuite/tools/expect
built as part of BYTECODE_OPTIONAL_TOOLS?
You are right that there is some redundancy here that needs to be
addressed, many thanks for having spotted it. I am not sure yet how to
best address it though, i.e. whether I should remove the dependency, or
rather adjust `configure.ac`.
One interesting thing to notice about `codegen` is that, at least for
the time being,
it's a bytecode tool but that is used only for native tests. Its
situation thus looks a bit odd to me. Wouldn't it make things a bit
clearer if we would build it as a native tool only, and then only when
the native compiler is enabled?
Then of course the question of how to build it remains. Ot me, having
the tools as dependencies of ocamltest looks slightly more meaningful
than adding them to generic list of tools. What's your take?
In Makefile:
> ocamltest/ocamltest$(EXE): OC_BYTECODE_LINKFLAGS += -custom
ocamltest/ocamltest$(EXE): ocamlc ocamlyacc ocamllex
-ocamltest.opt: ocamltest/ocamltest.opt$(EXE) testsuite/lib/testing.cmxa
+ocamltest.opt: ocamltest/ocamltest.opt$(EXE) \
+ testsuite/lib/testing.cmxa $(asmgen_OBJECT) testsuite/tools/codegen$(EXE)
Similarly, isn't this part of the optional tools variable?
I see the response as a matter of taste, as I tried to explain above,
but let's see.
In [6]configure.ac:
> @@ -2314,7 +2319,8 @@ AS_CASE([$enable_ocamltest,OCAML__DEVELOPMENT_VERSION],
ocamltest_target=ocamltest
ocamltest_opt_target=ocamltest.opt
ocamltest='ocamltest'
- ],
+ testsuite_tools='testsuite/tools/codegen testsuite/tools/expect'
As a further optimisation (I think) codegen is only needed if the
native compiler is being built
Yes, I think it's what I tried to do by adding
`testsuite/tools/codegen$(EXE)` as a dependency of `ocamltest.opt` in
the root Makefile but perhaps that's not clean enough?
|
4f5d0de to
ad79c5a
Compare
ad79c5a to
5fe3f8d
Compare
Contributor
Author
|
Giving it a second thought, I believe that the code in this PR is actually correct as it is. More specifically, regarding the discussions on where we should list the |
5fe3f8d to
5d843ab
Compare
Contributor
Author
|
I just pushed an update that takes into account @dra27's remark about With this, I hope I have addressed all the review comments done so far. |
5d843ab to
e1ba713
Compare
Fail with a clear error when trying to build ocamltest whereas it has been disabled at configure time
e1ba713 to
c20c1ff
Compare
Contributor
Author
|
Thanks! And thanks for the careful and supportive review, as usual!
|
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
2 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 merges
testsuite/{lib,tools}/Makefileinto the root Makefile and does two additional perhaps more interesting things:If ocamltest is enabled, all the support libraries and tools required to run certain tests are built, too. This means that any test can then be run by ocamltest directly without having to necessarily go through make to ensure that the required libraries and tools are built. This fulfulls a wish expressed some time ago by @Octachron and I am pleased to offer this modest gift to him. @gasche may also like this, and the coming item, too.
The PR makes it impossible to build ocamltest if it has not been enabled at configure time and gives an explicit error message explaining what is going on. This is an attempt to address Test failure when glibc has frame pointers #12587, more precisely a problem it revealed, see e.g. Test failure when glibc has frame pointers #12587 (comment) since if ocamltest is disabled at configuration time, this PR makes is then completely impossible to build it, even manually.