Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear the load path for expectation tests #1621

Merged
merged 3 commits into from Feb 26, 2018

Conversation

Projects
None yet
5 participants
@diml
Copy link
Member

diml commented Feb 20, 2018

To make sure the typer doesn't try to load installed cmi files.

@diml diml referenced this pull request Feb 20, 2018

Merged

Prefix the stdlib #1010

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Feb 20, 2018

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.

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 20, 2018

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

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Feb 20, 2018

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.

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 20, 2018

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

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Feb 20, 2018

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?

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 20, 2018

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.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Feb 20, 2018

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.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 20, 2018

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?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 20, 2018

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.)

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@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.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 20, 2018

$ git grep EXPECT_FLAGS | cat
ocamltest/ocaml_actions.ml:  let expect_flags = try Sys.getenv "EXPECT_FLAGS" with Not_found -> "" in
testsuite/makefiles/Makefile.expect:	  TERM=dumb $(EXPECT_TEST) $(EXPECT_FLAGS) -repo-root $(OTOPDIR) $$file && \
testsuite/makefiles/Makefile.expect:	  TERM=dumb $(EXPECT_TEST) $(EXPECT_FLAGS) -repo-root $(OTOPDIR) -principal \
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 20, 2018

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

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 20, 2018

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.)

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

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

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 26, 2018

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

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

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.

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 26, 2018

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.

@trefis

trefis approved these changes Feb 26, 2018

Copy link
Contributor

trefis left a comment

LGTM.

(Do you want to add a Changes entry?)

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Feb 26, 2018

Do you want to add a Changes entry?

done

@diml diml force-pushed the diml:fix-expect-tests-load-path branch from 80ecb24 to 76e573c Feb 26, 2018

@diml diml merged commit 70eb179 into ocaml:trunk Feb 26, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

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.)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

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)

@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented Feb 26, 2018

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.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.