Skip to content

Relocatable OCaml - test harness#175

Closed
dra27 wants to merge 103 commits intotrunkfrom
in-prefix-test
Closed

Relocatable OCaml - test harness#175
dra27 wants to merge 103 commits intotrunkfrom
in-prefix-test

Conversation

@dra27
Copy link
Copy Markdown
Owner

@dra27 dra27 commented Dec 15, 2024

This PR implements a test harness for Relocatable OCaml (ocaml/RFCs#53). The RFC defines "Relocatable" as meaning:

  1. The compiler binaries are identical regardless of the installation prefix (the --prefix argument passed to configure) or the working directory in which the compiler was built (the build path).
  2. The resulting compiler distribution can be used from any disk location on the system without any further alteration to the binaries.
  3. The resulting compiler distribution can be used from any disk location on the system without any further alteration to the user's shell environment.

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 that compiler works.

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 with ocamlrun). 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:

  1. it is central to this test that it runs on the compiler having been installed to the prefix it was configured with; and
  2. the test focuses more on the operation and execution of the programs than of the actual programs themselves, requiring more control over the exact commands executed to build the tests than ocamltest offers

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 #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:

  • The compilation tests are vastly simpler if they can use Ccomp.call_linker (I mean really, really, simpler - I tried!). However, in order to that, the test harness needs to be able to control slightly more precisely the value of Config.standard_library as interpreted by Ccomp.call_linker. Having tried various approaches, the least invasive to compiler-libs seems to be to generalise Compmisc.init_path via a new Compmisc.reinit_path. There are other ways of doing this, but this one I think is the simplest that doesn't involve duplicating code from utils/ccomp.ml directly in the test harness.
  • Test_ld_conf tests the handling by both ocamlrun and ocamlc of ld.conf. This test needs to be able the content of the search_path variable in Dll for which I have added a Dll.search_path function.
  • Both the test harness and one of the subsequent Relocatable PRs need to iterate over the code points of a UTF-8 string, and for this reason I've added Misc.Stdlib.String.to_utf_8_seq and, as always, I'm open to suggestions for a better name.
  • The test harness needs to be able to parse runtime-launch-info, so I've exposed Bytelink.read_runtime_launch_info and its associated types.
  • There are various parts of the tests for reproducibility which are clang-specific. The easiest way to do this seems to be to expose the ocaml_cc_vendor variable computed in configure as Config.cc_vendor

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 only! 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 cost. Note also that there are a few places where behaviour is not reachable or spelled out slightly obtusely: for example, the Default_ocamlc constructor includes an ignored launch_method value, 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 statically, rather than by probing the installation. In particular, this allows errors in the installation process to be detected (i.e. 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 the Config module - BINDIR and LIBDIR are passed on the command line, because Config.bindir and Config.standard_library won't have the same meaning in subsequent PRs. Especially given that the need to be able to use Ccomp.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 both 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 #175.

The harness code itself is structured with utility functions in module Harness and information derived about the C compiler and platform in Toolchain. Module Environment (and in particular Environment.run_process) encapsulates the configuration of the configuration of the compiler installation. Cmdline and Test_in_prefix drive the entire process with TestRelocation, TestBytecodeBinaries, TestDynlink, TestLinkModes,TestTopLevel and Test_ld_conf containing the implementations of the six tests.

@dra27 dra27 added no-change-entry-needed Causes the check for a Changes entry to be skipped for PRs CI: Skip testsuite Skip the testsuite runs on a PR labels Dec 15, 2024
@dra27 dra27 force-pushed the in-prefix-test branch 2 times, most recently from 8ef6b88 to 930e128 Compare December 15, 2024 18:52
@dra27 dra27 changed the base branch from trunk to cross-fixes December 18, 2024 13:41
@dra27 dra27 force-pushed the in-prefix-test branch 9 times, most recently from eac09ac to d72f770 Compare December 19, 2024 23:18
@dra27 dra27 changed the base branch from cross-fixes to trunk December 20, 2024 11:48
@dra27 dra27 force-pushed the in-prefix-test branch 2 times, most recently from f1e3683 to d365fb1 Compare December 20, 2024 15:24
@dra27 dra27 changed the title Workflow check Relocatable OCaml - test harness Dec 20, 2024
@dra27 dra27 added the relocatable PRs related to the Relocatable Compiler project label Dec 20, 2024
Comment on lines +1993 to +2006
testsuite/tools/test_in_prefi%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS)

testsuite/tools/test_in_prefix$(EXE): OC_BYTECODE_LINKFLAGS += -custom

testsuite/tools/test_in_prefi%: CAMLOPT = $(BEST_OCAMLOPT) $(STDLIBFLAGS)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
testsuite/tools/test_in_prefi%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS)
testsuite/tools/test_in_prefix$(EXE): OC_BYTECODE_LINKFLAGS += -custom
testsuite/tools/test_in_prefi%: CAMLOPT = $(BEST_OCAMLOPT) $(STDLIBFLAGS)
testsuite/tools/test_in_prefix%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS)
testsuite/tools/test_in_prefix$(EXE): OC_BYTECODE_LINKFLAGS += -custom
testsuite/tools/test_in_prefix%: CAMLOPT = $(BEST_OCAMLOPT) $(STDLIBFLAGS)

Why drop the leading O in OCAMLC and OCAMLOPT? or is it always spelled this way?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why drop the leading O in OCAMLC and OCAMLOPT? or is it always spelled this way?

@shindere definitely wants to change this (the names are just legacy from Caml Light and Caml Special Light!)... I think the idea was to wait until the Makefiles are all merged... renaming Makefile variables is of course a tricky process to get right (all the recursive calls, etc.)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makefile patterns cannot match the empty string, which is why the x is omitted - this rule is intended to match both testsuite/tools/test_in_prefix and testsuite/tools/test_in_prefix.opt and the pattern testsuite/tools/test_in_prefix% doesn't match the first one.

cf. the same trick in stdlib/Makefile:

ocaml/stdlib/Makefile

Lines 93 to 94 in f28bfbc

%-launch-info: %.info tmpheader.exe
@cat $^ >> $@

where the pattern is %-launch-info (to match both runtime-launch-info and target_runtime-launch-info)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting Makefile craziness... thanks!


#ifdef CAML_INTERNALS

#include <stdbool.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is including stdbool.h fine here w.r.t (the absence of) namespaces and C++ ABI compatibility because it's guarded by CAML_INTERNALS?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instinctively I'd say yes, but is there an immediately cleaner way I could put it? The other option would be just to use an int, of course 😉

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine, we're not guarding things under CAML_INTERNALS with extern "C" for C++ compat either.

Comment on lines +6 to +8
(e.g. /usr/bin and /usr/lib/ocaml share a common root of /usr, but /bin and
/lib/ocaml do not share a common root directory). The test needs to be able to
rename this common root directory by adding a suffix ".new". For this reasoa,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(e.g. /usr/bin and /usr/lib/ocaml share a common root of /usr, but /bin and
/lib/ocaml do not share a common root directory). The test needs to be able to
rename this common root directory by adding a suffix ".new". For this reasoa,
(e.g. `/usr/bin` and `/usr/lib/ocaml` share a common root of `/usr`, but `/bin` and
`/lib/ocaml` do not share a common root directory). The test needs to be able to
rename this common root directory by adding a suffix ".new". For this reason,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a thought (just to explain it), that putting these paths as code wasn't aiding readability (either rendered or as text) - i.e. I was finding "/usr/bin and /usr/lib/ocaml" easier to read (because each path is "introduced" by a slash anyway) than the version where the backticks are kinda getting in the way?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me :)

@shindere
Copy link
Copy Markdown

shindere commented Dec 27, 2024 via email

@dra27
Copy link
Copy Markdown
Owner Author

dra27 commented Jan 7, 2025

Markdown fixed; and...

@dra27
Copy link
Copy Markdown
Owner Author

dra27 commented Jan 7, 2025

... rebased

dra27 and others added 18 commits May 13, 2025 16:57
In native mode, same as Sys.executable_name, in bytecode, the path to
the interpreter executing Sys.executable_name, which may not be the same
from the same file.
…em` (ocaml#14028)

* Remove stale comments

* Remove dead constructor: Scoping_let_module

* Remove eqparsetree.ml
Update FlexDLL to 0.44 and reenable lib-dynlink-domains test on Windows
…b-musl

tests: native-debugger: sanitize GDB+musl output
Reviewed-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
…-variables

Fix ocamltest variable handling
[minor] use a record in Compenv.process_deferred_actions
@dra27 dra27 force-pushed the in-prefix-test branch from 9808a1e to 3924106 Compare May 15, 2025 13:39
nojb and others added 9 commits May 15, 2025 16:13
Add `Sys.runtime_executable` and `caml_sys_proc_self_exe`
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.
Expose the ocaml_cc_vendor variable computed in aclocal.m4
@dra27 dra27 force-pushed the in-prefix-test branch from 3924106 to d39460d Compare May 15, 2025 15:17
@dra27 dra27 closed this May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: Full matrix Full CI test matrix CI: Skip testsuite Skip the testsuite runs on a PR no-change-entry-needed Causes the check for a Changes entry to be skipped for PRs relocatable PRs related to the Relocatable Compiler project run-crosscompiler-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.