Relocatable OCaml - test harness#14014
Conversation
nojb
left a comment
There was a problem hiding this comment.
LGTM
I did not look at the test harness itself (second part of the PR).
driver/compmisc.ml
Outdated
| let init_path ?(auto_include=auto_include) ?(dir="") () = | ||
| let reinit_path ?(auto_include=auto_include) | ||
| ?(standard_library=Config.standard_library) | ||
| ?(dir="") () = |
There was a problem hiding this comment.
Why not just add the optional argument to init_path and call it a day (instead of adding reinit_path)?
There was a problem hiding this comment.
I was nervous about adding an optional argument that was just for a test harness to a compiler-libs function which is called by user-code. Quite happy just to add ?standard_library to init_path if you think I'm being overly paranoid!
There was a problem hiding this comment.
I don't see much difference with adding the ~standard_library argument here and the existing ~dir, so yes, I would add it here and avoid adding a new function.
| Seq.Cons(u, to_utf_8_seq b (i + Uchar.utf_decode_length next)) | ||
|
|
||
| let to_utf_8_seq s = to_utf_8_seq (Bytes.unsafe_of_string s) 0 | ||
|
|
There was a problem hiding this comment.
We could consider adding this to the standard library :)
There was a problem hiding this comment.
Definitely! My only reticence with it is that the Standard Library would want a family of these
There was a problem hiding this comment.
Yes, clearly. Not to be done here of course!
| concatenated with the bytecode. So, if the attempt with argv[0] | ||
| failed, it is worth trying again with executable_name. */ | ||
| if (fd < 0 && (proc_self_exe = caml_executable_name()) != NULL) { | ||
| if (fd < 0 && proc_self_exe != NULL) { |
There was a problem hiding this comment.
Somewhat orthogonal, but I found this comment extremely hard to understand because of its placement: in fact both the code on 485 and the one below concern the same situation, that of -custom built bytecode executables. But due to the placement of the comment in between the two chunks of code, it looks like only one of them concerns that situation. If you agree with this understand, would you mind putting both chunks of code together and moving the comment either before or after both of them?
There was a problem hiding this comment.
(just for context, this part of the diff comes from #13728)
It's definitely confusing, and there's a lurking (very) minor security bug around the order in which things are done for -custom - this part of the code churns a lot in the second relocatable PR (cf. dra27@8f06c3d, which fixes the bug, but there are subsequent commits where this code churns a lot). Would it be OK to "hold that thought" for the later PR? 😊
There was a problem hiding this comment.
Yes, of course. Thanks for the quick response.
Up to you, but note that strictly speaking you do not need to wait. The reason #13728 is waiting is because it exposes the |
I'd debated it, but I imagined that Damien had been more interested in checking this first commit (the plumbing) than the name in the Stdlib! It's moot now, though, as Gabriel has approved it, too. |
0d482be to
07363c3
Compare
OlivierNicole
left a comment
There was a problem hiding this comment.
I reviewed commits 1 through 5 and only started on testsuite/in_prefix, but here is a first batch of (minor) comments.
|
Thank you both - revised version pushed (it's on the same base commit, so the "Compare" button works) |
|
Some of your typo fixes are part of the "Plumb the in-prefix tests into CI" commit, not sure that’s why you intended to do. |
OlivierNicole
left a comment
There was a problem hiding this comment.
I’ve read through the last two commits adding the tests and plumbing them into the CI.
As suggested, I have not read with the scrupulousness that I would have taken for the compiler code, where we want to have confidence that every function has the intended behaviour. Rather, I read through the (well-written and pleasant to read!) code, trying to understand the big picture of the test harness, and to stay alert for things that look strange (it wasn’t always easy because there is really a lot of code). I didn’t find such things.
Approving. I only have minor nitpicks, most of them typos. Since you expressed that modifying this PR can be costly, feel free to fix them in another PR.
Indeed it wasn't!! 😱 I feel as "Mr. Fussy", and have immediately straightened my bent grass (or in this case, misplaced fixup commit). Just looking at the rest, thank you! |
|
Thank you very much, @OlivierNicole - I think I have addressed everything, apart from one question.
No, this is all good - I was more meaning that wider "it might be better architected this way" moving or refactoring functions would be very painful because of changes to those functions in subsequent commits. |
|
My comments have been addressed. |
Previously, Compmisc.init_path initialised the load path using Config.standard_library, but this can now be altered via an optional ?standard_library argument. This is used internally when testing compiler installations in order to allow Ccomp.call_linker to be used.
Provides a copy of the DLL search path for the test harness.
c0affeb to
6117a96
Compare
Expose the ocaml_cc_vendor and shebangscripts variables computed in aclocal.m4
|
Anything left to do here before merging? The PR already has two approvals. |
|
Not that I can see, I think that you or @OlivierNicole or @dra27 should go ahead and merge. (I could do it as well, but I want to encourage other maintainers to merge when the time is right.) |
This PR implements a test harness for Relocatable OCaml (ocaml/RFCs#53). The RFC defines "Relocatable" as meaning:
--prefixargument passed toconfigure) or the working directory in which the compiler was built (the build path).Rather than building two compilers, the first property can be checked to a sufficient degree for Relocatable OCaml by searching the installed artefacts for prefix and build path. The second and third properties can be simultaneously checked by moving the compiler from prefix to a new location, and then running a series of tests to ensure that the compiler works in its new location.
Obviously, at present OCaml cannot be configured in a way which satisfies these properties, but it is of course possible to move a compiler to a new location and change the way the compiler is invoked in order to make it work. The harness in this PR implements this approach. Everything is thus tested with a combination of known failures and shims (such as setting
OCAMLLIB, or explicitly invoking bytecode binaries withocamlrun). The three further PRs which make up the proposed changes for Relocatable OCaml are based on top of this PR, and therefore update this test harness with the revised behaviour.There are two key differences between this test harness and the main ocamltest-based testsuite:
A consequence of the "in-prefix" part is that this is not a test that should be run by default and the fact it needs to operate outside the build tree has led to an additional harness, rather than additional features in ocamltest.
The harness itself has revealed various bugs not related to Relocatable OCaml at all (cf. ocaml/flexdll#146, #13496, #13520, #13638, #13692 and #13693, in addition to a fault in the partial linker alluded to in #13692 which will be fixed in a subsequent PR)
The tests performed are covered in
testsuite/in_prefix/README.md.In terms of review, the PR includes the first commit of #13728, which means all the changes in
runtime/can be ignored (as they all come from that commit). The next five commits alter the compiler:Ccomp.call_linker(I mean really, really, simpler - I tried!). However, in order to do that, the test harness needs to be able to control slightly more precisely the value ofConfig.standard_libraryas interpreted byCcomp.call_linker. Having tried various approaches, the least invasive to compiler-libs seems to be to generaliseCompmisc.init_pathvia a newCompmisc.reinit_path. There are other ways of doing this, but this one I think is the simplest that doesn't involve duplicating code fromutils/ccomp.mldirectly in the test harness.Test_ld_conftests the handling by bothocamlrunandocamlcofld.conf. This test needs to be able to read the content of thesearch_pathvariable inDllfor which I have added aDll.search_pathfunction.string, and for this reason I've addedMisc.Stdlib.String.to_utf_8_seq(as always, I'm open to suggestions for a better name...), rather than having the function in the harness itself.runtime-launch-info, so I've exposedBytelink.read_runtime_launch_infoand its associated types.ocaml_cc_vendorvariable computed inconfigureasConfig.cc_vendor, as it has a well-defined form (which I've documented in the process).Those opening commits clearly change the compiler, and need to be reviewed as such. The last two alter the testsuite only, and I'd beseech be reviewed from a clarity perspective, rather than as nittily as the main codebase requires! In particular, I have 12 branches based on top of this all of which alter this test harness, so please bear in mind that while adjustments are of course possible, they come at a bigger cost than just editing this PR. Note also that there are a few places where behaviour is not reachable or spelled out slightly obtusely. For example, the
Default_ocamlcconstructor includes an ignoredlaunch_methodvalue, and several functions have unused parameters escaped with_. These are all altered in subsequent PRs where the reason for having this differentiation becomes (hopefully) clearer.As far as possible, the harness learns of the configuration of the compiler statically, rather than by probing the installation. In particular, this allows errors in the installation process to be detected (e.g. if the toplevel test simply tested all the .cma files it found in libdir, it would not catch the failure to have installed one of them - hence
$(OTHERLIBRARIES)is passed on the command line instead). Most things are statically placed in theConfigmodule -BINDIRandLIBDIRare passed on the command line, becauseConfig.bindirandConfig.standard_librarywon't have the same meaning in subsequent PRs. Especially given the need to be able to useCcomp.call_linker, I've not written this test to be used in a cross-compiled manner, by which I mean it can test an installation which is a cross compiler, but the test harness must itself be compiled with the same (host) configuration as the compiler it's testing.The final commit plumbs the tests into CI - it (should be!) passing on GitHub Actions here and has also passed precheck (although I can't at present schedule a run of this precise version of the PR, as precheck seems to be down). It's also passing an even wider text matrix including Cygwin and multiple different shebang/executable/static/minimal tests in dra27#175.
The harness code itself is structured with utility functions in module
Harnessand information derived about the C compiler and platform inToolchain. ModuleEnvironment(and in particularEnvironment.run_process) encapsulates the configuration of the configuration of the compiler installation.CmdlineandTest_in_prefixdrive the entire process withTestRelocation,TestBytecodeBinaries,TestDynlink,TestLinkModes,TestTopLevelandTest_ld_confcontaining the implementations of the six tests.