I'm not sure I quite understand how this works.
I was surprised to see you clear Config.load_path after having reading the cmdline arguments, does that mean that -Is get ignored?

It also seems like there is no call to Compmisc.init_path appart from the one at toplevel in toplevel, which I would assume is run before the cmdline args are read; again it seems that -Is get ignored.

Apparently it doesn't matter, but it might come back to bite us in the future.

I was surprised to see you clear Config.load_path after having reading the cmdline arguments, does that mean that -Is get ignored?

Yes. Looking at the code they were already ignored before actually.

It also seems like there is no call to Compmisc.init_path appart from the one at toplevel in toplevel,

Where is it? I guess that's the one responsible for adding the stdlib dir

I meant "except for the one at toplevel in Toploop"; and indeed that'll be the one responsible for adding Config.standard_library to the loadpath.

Ah, I missed the let _ =... in toploop...

So, I think you might want to remove the clearing of Config.loadpath as well as the

List.iter [ "stdlib" ] ~f:(fun s ->                     
  Topdirs.dir_directory (Filename.concat !repo_root s));

and instead want to call

Compmisc.init_path ~dir:(Filename.concat !repo_root "stdlib") <some boolean>

at that point.

Does that sound right to you?

That seems OK, with a Clflags.no_std_include := true before to make sure we don't include the stdlib dir. However, if I read Compmisc.init_path correctly, that means that <repo-root>/stdlib will be at the beginning of Config.load_path, while the stdlib dir is usually the last path.

Indeed, then I guess you want to add it to Clflags.include_dirs before parsing the cmdline args, and then call Compmisc.init_path without passing the optional flags.

Yes, that seems more in line with the rest of the tools. What should I modify to pass these flags to the expect_test tool?

Note that all ways to invoke except_test already honor the EXPECT_FLAGS environment variable, so you should be able to experiment with support for -nostdlib and -I ... (which I agree is a nice solution, thanks @shindere) without any ocamltest-side change. (Of course, having ocamltest pass these by default when invoking expect tests would be best.)

@gasche: "Note that all ways to invoke except_test already honor the EXPECT_FLAGS environment
variable": which ways did you have in mind, actually? I grepped for
EXPECT_FLAGS in the repo but it seems only used by the legacy makefile which
is itself no longer used. But are there perhaps tools external to the OCaml
repository that do call expect_tool?
Please enter your comment below.

$EXPECT_FLAGS is passed to expect_test by both the Makefile and ocamltest.

Passing the flag on the command-line when invoking the tool means that the content is parsed by the shell. If the tool wanted to interpret this variable themselves it would have to implement shell-style command-line parsing itself, which is painful -- this is why the OCaml compiler for example implements a different format in OCAMLPARAM, OCAMLRUNPARAM etc.

On the other hand, ocamltest supports flags = "..." in its header for toplevel tests, and it would certainly be reasonable to respect the same interface for expect-style tests -- in addition to, or replacement of, the EXPECT_FLAGS support.

(Note that honoring EXPECT_FLAGS is useful for cross-cutting concerns such as the one we are presently discussing here: it lets Jérémie experiment more easily without requiring ocamltest-side changes, before a long-term solution which doesn't use EXPECT_FLAGS is implemented.)

Gentle Monday ping: unit tests are still broken on trunk.

Ah, I thought this was just an improvement...

The test breakage only shows up if you don't install the system before testing (not installing is standard) and you have a meaningful installation path that already contains stuff (from a previous install test, for example; this may be non-standard workflow). This means that there is a workaround (always install the distribution before testing), so this is not critical; but it breaks assumptions that some people's workflow (typically mine) rely on.

I wouldn't mind fixing it myself, but I figured that you (@diml) would be more familiar with the toplevel codebase so that it would probably be sensibly easier for you. If you don't have resources to work on it now, I don't mind giving it a try later in the week.

Ok, that's annoying indeed. I pushed another commit that does the following:

• the expect_tool interpret command line arguments in the same way as other tools do
• additionally, if we pass -repo-root, it replaces the stdlib dir by <repo-root>/stdlib

So this should fix the issue, we should be able to pass arbitrary flags to the tool and we don't need to modify anything else at this point.

Moving forward, I think one thing what would be even better is to push this feature in the toplevel itself, this way it wouldn't be a second class tool that we might forget to update. Though that's a bit more work.

Do you want to add a Changes entry?

done

Thanks! With this PR merged and a stale install-directory on my machine, all tests now pass.

(I observed a transient failure with tests/tool-ocamldoc-html/Module_whitespace; I suspect it's just a transient issue but I'll have a look.)

The failure with tool-ocamldoc-html/Module_whitespace is reproducible: the test file contains Stdlib.Set.Make in the output, and running it with a stale installation directly produces Set.Make instead, making the test fail. There seems to be another destdir-robustness issue with ocamldoc. (cc @Octachron)

If it needs to be fixed soon, the test could be rewritten to define its own functor rather than use Set.Make and I will have a look at the root issue later this week.

I think we can wait for you (or someone else) to have time to look at it.