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

ocamlbuild rules for -principal, -strict-sequence and -short-paths #6060

Closed
vicuna opened this issue Jun 30, 2013 · 9 comments
Closed

ocamlbuild rules for -principal, -strict-sequence and -short-paths #6060

vicuna opened this issue Jun 30, 2013 · 9 comments

Comments

@vicuna
Copy link

vicuna commented Jun 30, 2013

Original bug ID: 6060
Reporter: @avsm
Status: resolved (set by @damiendoligez on 2017-03-01T15:53:48Z)
Resolution: suspended
Priority: normal
Severity: feature
Fixed in version: 4.01.0+dev
Category: -for ocamlbuild use https://github.com/ocaml/ocamlbuild/issues
Tags: patch
Related to: #6129
Monitored by: @gasche kerneis

Bug description

These are all referenced in RWO, so having ocamlbuild rules for the options makes demonstrating them easier. Patch is against 4.01.
https://github.com/avsm/ocaml/commit/33aff15c0255d823bac314b2acb9ecab1af961e9.patch

From 33aff15 Mon Sep 17 00:00:00 2001
From: Anil Madhavapeddy anil@recoil.org
Date: Sun, 30 Jun 2013 18:48:21 +0100
Subject: [PATCH] Add some missing compiler options to ocamlbuild.

  • -short-paths is activated via the "short_paths" tag
  • -principal is activated via the "principal" tag
  • -strict_sequence now has a "strict_sequence" tag to alias the
    "strict-sequence" tag that was already there, to follow the
    convention of command-line options having dashes replaced by
    underscores. It's easy to mess this up since incorrect tags
    are silently ignored by ocamlbuild.

Add test cases that check if principal and strict-sequence have
been passed, and tweak the test suite slightly to make it easier
to match on failing_msg output.

ocamlbuild/ocaml_specific.ml | 5 +++++
ocamlbuild/testsuite/level0.ml | 19 +++++++++++++++++++
ocamlbuild/testsuite/ocamlbuild_test.ml | 4 +++-
3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ocamlbuild/ocaml_specific.ml b/ocamlbuild/ocaml_specific.ml
index a5873ae..00f1053 100644
--- a/ocamlbuild/ocaml_specific.ml
+++ b/ocamlbuild/ocaml_specific.ml
@@ -553,10 +553,14 @@ flag ["ocaml"; "link"; "byte"; "output_obj"] (A"-output-obj");;
flag ["ocaml"; "dtypes"; "compile"] (A "-dtypes");;
flag ["ocaml"; "annot"; "compile"] (A "-annot");;
flag ["ocaml"; "bin_annot"; "compile"] (A "-bin-annot");;
+flag ["ocaml"; "short_paths"; "compile"] (A "-short-paths");;
+flag ["ocaml"; "short_paths"; "infer_interface"] (A "-short_paths");;
flag ["ocaml"; "rectypes"; "compile"] (A "-rectypes");;
flag ["ocaml"; "rectypes"; "infer_interface"] (A "-rectypes");;
flag ["ocaml"; "rectypes"; "doc"] (A "-rectypes");;
flag ["ocaml"; "rectypes"; "pack"] (A "-rectypes");;
+flag ["ocaml"; "principal"; "compile"] (A "-principal");;
+flag ["ocaml"; "principal"; "infer_interface"] (A "-principal");;
flag ["ocaml"; "linkall"; "link"] (A "-linkall");;
flag ["ocaml"; "link"; "profile"; "native"] (A "-p");;
flag ["ocaml"; "link"; "program"; "custom"; "byte"] (A "-custom");;
@@ -594,6 +598,7 @@ let ocaml_warn_flag c =
List.iter ocaml_warn_flag ['A'; 'C'; 'D'; 'E'; 'F'; 'K'; 'L'; 'M'; 'P'; 'R'; 'S'; 'U'; 'V'; 'X'; 'Y'; 'Z'];;

flag ["ocaml"; "compile"; "strict-sequence"] (A "-strict-sequence");;
+flag ["ocaml"; "compile"; "strict_sequence"] (A "-strict-sequence");;

flag ["ocaml"; "doc"; "docdir"; "extension:html"] (A"-html");;
flag ["ocaml"; "doc"; "docdir"; "manpage"] (A"-man");;
diff --git a/ocamlbuild/testsuite/level0.ml b/ocamlbuild/testsuite/level0.ml
index 52739e4..ff8c4db 100644
--- a/ocamlbuild/testsuite/level0.ml
+++ b/ocamlbuild/testsuite/level0.ml
@@ -167,4 +167,23 @@ test "OutputObj"
~tree:[T.f "hello.ml" ~content:"print_endline "Hello, World!""]
~targets:("hello.byte.o",["hello.byte.c";"hello.native.o"]) ();;

+test "StrictSequenceFlag"

  • ~description:"-strict_sequence tag"
  • ~tree:[T.f "hello.ml" ~content:"let () = 1; ()";
  •     T.f "_tags" ~content:"true: strict_sequence\n"]
    
  • ~options:[`quiet]
  • ~failing_msg:"File "hello.ml", line 1, characters 9-10:
    +Error: This expression has type int but an expression was expected of type
  •     unit\nCommand exited with code 2."
    
  • ~targets:("hello.byte",[]) ();;

+test "PrincipalFlag"

  • ~description:"-principal tag"
  • ~tree:[T.f "hello.ml" ~content:"type s={foo:int;bar:unit} type t={foo:int} let f x = x.bar;x.foo";
  •     T.f "_tags" ~content:"true: principal\n"]
    
  • ~options:[`quiet]
  • ~failing_msg:"File "hello.ml", line 1, characters 61-64:
    +Warning 18: this type-based field disambiguation is not principal."
  • ~targets:("hello.byte",["hello.native"]) ();;

run ~root:"_test";;
diff --git a/ocamlbuild/testsuite/ocamlbuild_test.ml b/ocamlbuild/testsuite/ocamlbuild_test.ml
index 3552a40..1b76ea0 100644
--- a/ocamlbuild/testsuite/ocamlbuild_test.ml
+++ b/ocamlbuild/testsuite/ocamlbuild_test.ml
@@ -442,11 +442,13 @@ let run ~root =
Printf.printf "\x1b[0;31m\x1b[1m[FAILED]\x1b[0m \x1b[1m%-20s\x1b[0;33m%s.\n%!" name
(Printf.sprintf "Command '%s' with error code %n output written to %s" cmd n log_name);
| Some failing_msg ->

  •    let starts_with_plus s = String.length s > 0 && s.[0] = '+' in
    
  •    let lines = List.filter (fun s -> not (starts_with_plus s)) lines in
       let msg = String.concat "\n" lines in
       if failing_msg = msg then
         Printf.printf "\x1b[0;32m\x1b[1m[PASSED]\x1b[0m \x1b[1m%-20s\x1b[0;36m%s.\n%!" name description
       else
    
  •      Printf.printf "\x1b[0;31m\x1b[1m[FAILED]\x1b[0m \x1b[1m%-20s\x1b[0;33m%s.\n%!" name ((Printf.sprintf "Failure with not matching message: %s") msg)
    
  •      Printf.printf "\x1b[0;31m\x1b[1m[FAILED]\x1b[0m \x1b[1m%-20s\x1b[0;33m%s.\n%!" name ((Printf.sprintf "Failure with not matching message:\n%s\n!=\n%s\n") msg failing_msg)
     end;
    
    | _ ->
    let errors = List.concat (List.map (Match.match_with_fs ~root:full_name) matching) in
    --
    1.8.1.6

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 30, 2013

Comment author: meyer

Thanks for the patch! Applied as r13859.

I really like that the tests come with the patch! but in future please remember also to update the Changes file.

@vicuna
Copy link
Author

vicuna commented Jul 1, 2013

Comment author: @avsm

Thanks, I'll add a Changes entry in future patches. Would you mind merging this into 4.01 too?

@vicuna
Copy link
Author

vicuna commented Jul 4, 2013

Comment author: @avsm

There was a small typo in the infer_interface portion of the patch (just the one testcase I didn't cover, ironically). Could the following please be committed to trunk and 4.01 to fix the "ocamlbuild -tag short_paths foo.inferred.mli" target, please?

https://github.com/avsm/ocaml/commit/ecc1a3a8ecd53407997805c29a8c569143833465.patch

From ecc1a3a Mon Sep 17 00:00:00 2001
From: Anil Madhavapeddy anil@recoil.org
Date: Thu, 4 Jul 2013 14:47:55 +0100
Subject: [PATCH] fix typo in infer_interface tag: the only valid command-line
is -short-paths


ocamlbuild/ocaml_specific.ml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ocamlbuild/ocaml_specific.ml b/ocamlbuild/ocaml_specific.ml
index 00f1053..dc4d8dc 100644
--- a/ocamlbuild/ocaml_specific.ml
+++ b/ocamlbuild/ocaml_specific.ml
@@ -554,7 +554,7 @@ flag ["ocaml"; "dtypes"; "compile"] (A "-dtypes");;
flag ["ocaml"; "annot"; "compile"] (A "-annot");;
flag ["ocaml"; "bin_annot"; "compile"] (A "-bin-annot");;
flag ["ocaml"; "short_paths"; "compile"] (A "-short-paths");;
-flag ["ocaml"; "short_paths"; "infer_interface"] (A "-short_paths");;
+flag ["ocaml"; "short_paths"; "infer_interface"] (A "-short-paths");;
flag ["ocaml"; "rectypes"; "compile"] (A "-rectypes");;
flag ["ocaml"; "rectypes"; "infer_interface"] (A "-rectypes");;
flag ["ocaml"; "rectypes"; "doc"] (A "-rectypes");;

1.8.1.6

@vicuna
Copy link
Author

vicuna commented Jul 15, 2013

Comment author: @avsm

Could someone with dev access close this? The followup patch has been committed to both branches.

@vicuna
Copy link
Author

vicuna commented Aug 6, 2013

Comment author: @gasche

On my machine, the PrincipalFlag test fails, and I'm not sure whether it's a problem on my end or in the testsuite. The error I get is the following:

[FAILED] PrincipalFlag Failure with not matching message:
File "hello.ml", line 1, characters 61-64:
Warning 18: this type-based field disambiguation is not principal.
File "hello.ml", line 1, characters 61-64:
Warning 18: this type-based field disambiguation is not principal.
!=
File "hello.ml", line 1, characters 61-64:
Warning 18: this type-based field disambiguation is not principal.
.

The testsuite tries to build both test.byte and test.native, so I find it natural that the error message is reported twice by ocamlbuild (once for each target). If I comment out the second target, the test passes.

Anil, could you also comment on what the start_with_plus business is about? Is it related to terminal codes somehow? Or classic-display? I don't understand what its purpose is, and I'm wondering whether it couldn't make some tests fail by filtering out genuine output.

@vicuna
Copy link
Author

vicuna commented Aug 13, 2013

Comment author: kerneis

As far as I understand it, start_with_plus is to filter out the lines in classic-display which contain the actual compiler invocation.

@vicuna
Copy link
Author

vicuna commented Aug 13, 2013

Comment author: kerneis

I thought about this a bit more, and in fact it would make much more sense to simply try and match any subset of the output against failing_msg, rather than the full output. We certainly do not want to rely on the exact ordering of rules printed out by ocamlbuild for a test about failing resolution (and I'm about to propose such a test for #5185).

Here is attached a patch proposal to implement this solution.

The alternative would be to make failing_msg itself a regexp, but it's more error-prone.

@vicuna
Copy link
Author

vicuna commented Aug 13, 2013

Comment author: @gasche

I feel uneasy about matching only a subset of the output. It looks like it would make it possible to not detect problematic changes in behavior. The current statu quo, which is that tests have to be tweaked when the behavior of the compiler changes, seems better: we have some false test failures that do not correspond to actual regression, but it is easy to detect those on a case-by-case basis.

Something we could consider doing in providing an abstraction layer over the actual errors and warnings that ocamlbuild emits, either on the testsuite side (re-parsing error messages and returning an easy-to-match-over representation, as we have an abstraction over tags etc.), or directly in the tool itself (providing a different mode for easy-to-work-over-programmaticaly error reports; this has been discussed for the OCaml compiler itself to ease interaction with IDEs). That's more work than I can afford right now, so it's more a meditation for the future.

(I think we could profitably postpone this testsuite issue until the next release, as it will not affect users, contrarily to some PR that could benefit from more attention such that #5185, on which you are already working on, or #5895, #5680 or #6087. That said, I appreciate this contribution and the fact that you take testing seriously.)

@vicuna
Copy link
Author

vicuna commented Mar 1, 2017

Comment author: @damiendoligez

ocamlbuild is now a separate project that lives on GitHub.
PR transferred to ocaml/ocamlbuild#197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant