Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006060OCamlOCamlbuild (the tool)public2013-06-30 19:542014-01-17 15:38
Reporteravsm 
Assigned To 
PrioritynormalSeverityfeatureReproducibilityalways
StatusfeedbackResolutionreopened 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version4.01.0+dev 
Summary0006060: ocamlbuild rules for -principal, -strict-sequence and -short-paths
DescriptionThese 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 33aff15c0255d823bac314b2acb9ecab1af961e9 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
Tagspatch
Attached Filespatch file icon 0001-ocamlbuild_test-permissive-match-of-failure_msg.patch [^] (2,420 bytes) 2013-08-13 10:30 [Show Content]

- Relationships
related to 0006129resolvedgasche PrincipalFlag test broken for ocamlbuild 

-  Notes
(0009656)
meyer (developer)
2013-07-01 01:28

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.
(0009661)
avsm (reporter)
2013-07-01 13:39

Thanks, I'll add a Changes entry in future patches. Would you mind merging this into 4.01 too?
(0009692)
avsm (reporter)
2013-07-04 15:50

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 ecc1a3a8ecd53407997805c29a8c569143833465 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
(0009777)
avsm (reporter)
2013-07-15 12:54

Could someone with dev access close this? The followup patch has been committed to both branches.
(0010137)
gasche (developer)
2013-08-06 20:29

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.
(0010164)
kerneis (reporter)
2013-08-13 07:03

As far as I understand it, start_with_plus is to filter out the lines in classic-display which contain the actual compiler invocation.
(0010165)
kerneis (reporter)
2013-08-13 10:29

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 PR#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.
(0010168)
gasche (developer)
2013-08-13 11:28
edited on: 2013-08-13 11:29

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 PR#5185, on which you are already working on, or PR#5895, PR#5680 or PR#6087. That said, I appreciate this contribution and the fact that you take testing seriously.)


- Issue History
Date Modified Username Field Change
2013-06-30 19:54 avsm New Issue
2013-07-01 01:19 meyer Assigned To => meyer
2013-07-01 01:19 meyer Status new => assigned
2013-07-01 01:28 meyer Note Added: 0009656
2013-07-01 13:39 avsm Note Added: 0009661
2013-07-04 15:50 avsm Note Added: 0009692
2013-07-15 12:54 avsm Note Added: 0009777
2013-07-16 01:09 gasche Status assigned => resolved
2013-07-16 01:09 gasche Resolution open => fixed
2013-07-26 10:39 meyer Status resolved => closed
2013-07-26 10:39 meyer Fixed in Version => 4.01.0+dev
2013-08-06 20:29 gasche Assigned To meyer =>
2013-08-06 20:29 gasche Note Added: 0010137
2013-08-06 20:29 gasche Status closed => feedback
2013-08-06 20:29 gasche Resolution fixed => reopened
2013-08-12 21:08 gasche Relationship added related to 0006129
2013-08-13 07:03 kerneis Note Added: 0010164
2013-08-13 10:29 kerneis Note Added: 0010165
2013-08-13 10:30 kerneis File Added: 0001-ocamlbuild_test-permissive-match-of-failure_msg.patch
2013-08-13 11:28 gasche Note Added: 0010168
2013-08-13 11:29 gasche Note Edited: 0010168 View Revisions
2014-01-17 15:38 doligez Tag Attached: patch


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker