type_conv-108.07 breakage

[Anil Madhavapeddy]

I've been porting some of our syntax extensions to the new type_conv (from 108.00.02 -> 108.07), and the Dyntype extension is broken because of some extra bits that are generated:

For a simple:

# type t = int with value;;

This diff happens between the output of 108.00.02 and 108.07.00:

--- simple-108.00.02.ml 2012-09-29 21:29:42.000000000 -0700
+++ _build/simple_value_test.ml 2012-09-29 21:30:04.000000000 -0700
@@ -22,6 +22,12 @@
then Dyntype.Value.Rec ((("t", __id__), __x__))
else Dyntype.Value.Ext ((("t", (gen_t_id ~id_seed __x3__)), __x__))

+let _ = t
+and _ = t
+and _ = t
+and _ = ref
+and _ = value_of_t
+
let (t_of_value : Dyntype.Value.t -> t) =
let module Deps =
struct
@@ -73,4 +79,8 @@
raise (Deps.Runtime_error (("Var/Rec/Ext", __x__))))
in t_of_value_aux { Deps.t = []; }

+let _ = t
+and _ = t
+and _ = t_of_value
+

These are unbound, and so compilation breaks... what are they for?

-anil

[Yury Sulsky]

Hi Anil,

I'll take a look at the dyntype extension to see how type_conv is broken, but the point of these assignments is to avoid ocaml 4's unused value warnings by from generated code. Apparently, type_conv thinks that values called "t", "ref", "t_of_value", and "value_of_t" have been generated by this extension.

[Anil Madhavapeddy]

Hi Yury,

I got a little lost in the new type_conv control flow (objects, why is it always objects!), but it looks like the bug is at:

class vars_of = object
inherit fold as super
val vars = []
method vars = vars
method ident = function
| <:ident< $lid:v$ >> -> {< vars = v :: vars >}
| ident -> super#ident ident
end
let lids_of_patt patt =
((new vars_of)#patt patt)#vars

This extracts out all the identifiers in a pattern binding. In the case of dyntype, we generate output like:

let (value_of_t : ?id_seed: (Int64.t ref) -> t -> Dyntype.Value.t) =
<snip> in
fun ?id_seed t =
<snip>

I've simplified the Dyntype generation to be simply:

let value_of_t, value_of_foo =
(fun ?id_seed t -> <...>), (fun ?id_seed t -> <...>)

and it works fine now. The changeset is:
https://github.com/mirage/dyntype/commit/3569269c8cad3f013151fb1b5d89cf76981f1df1
and is present in dyntype-0.9.0.

Where would you like Core bugs such as this type_conv one to be reported? The bitbucket issue tracker?

-anil

[Yury Sulsky]

Hi Anil,

This was definitely a bug in type-conv. Valentin has a fix for this:

diff --git a/base/type_conv/lib/pa_type_conv.ml b/base/type_conv/lib/pa_type_conv.ml
--- a/base/type_conv/lib/pa_type_conv.ml
+++ b/base/type_conv/lib/pa_type_conv.ml
@@ -371,10 +371,11 @@
       | <:ctyp@loc< +'$var$ >> | <:ctyp@loc< -'$var$ >> -> <:ctyp@loc< '$var$ >>
       | tp -> tp))#ctyp
 
-  class vars_of = object
+  class vars_of = object (self)

     inherit fold as super
     val vars = []
     method vars = vars

+    method! ctyp _ = self

     method ident = function
     | <:ident< $lid:v$ >> -> {< vars = v :: vars >}
     | ident -> super#ident ident

I'd like to get a quick bugfix out the door that includes this and more portable use of mktemp in base/core/lib/discover.sh.

[Anil Madhavapeddy]

Great! The workaround was pretty straightforward, so I'll leave the simpler code generation in place.

Let me know when the bugfix is out, and we'll update OPAM. Can your release system issue only a bumped version number for Core and Type_conv, instead of a complete set of new packages with new versions? This is a good test of the constraint system, as our constraints are probably way too specific (="108.07.00").

-a

[Markus Mottl]

Hi Yuri,

On Mon, Oct 1, 2012 at 1:37 PM, Yury Sulsky <yury....@gmail.com> wrote:
> This was definitely a bug in type-conv. Valentin has a fix for this:

It seems your latest version of type_conv has diverged already quite a
lot from what's in the Core development tree or even patch queue. I'm
not sure what your release plans are for the future, but I find it
rather hard to merge or even keep track of changes that happen in
tarball releases but not in the development tree. What are your
future plans for the development process and release cycles? I'd hope
we'll see more activity in the development tree or at least patch
queue.

Cheers,
Markus

--
Markus Mottl http://www.ocaml.info markus...@gmail.com

[Anil Madhavapeddy]

On 1 Oct 2012, at 11:28, Markus Mottl <markus...@gmail.com> wrote:

> Hi Yuri,
>
> On Mon, Oct 1, 2012 at 1:37 PM, Yury Sulsky <yury....@gmail.com> wrote:
>> This was definitely a bug in type-conv. Valentin has a fix for this:
>
> It seems your latest version of type_conv has diverged already quite a
> lot from what's in the Core development tree or even patch queue. I'm
> not sure what your release plans are for the future, but I find it
> rather hard to merge or even keep track of changes that happen in
> tarball releases but not in the development tree. What are your
> future plans for the development process and release cycles? I'd hope
> we'll see more activity in the development tree or at least patch
> queue.

I had a quick chat with Yaron about this at OUD. It seems that the best
way to export changes from the JSC dev trees is via tarballs (that lose
history). However, if we do not need patches to go *bidirectionally*, then
the Mercurial export infrastructure could be significantly simplified.

I propose something along these lines:

- JSC releases tarballs that are similar to the current ones, but with
preX releases so we can see some changes before they're committed to
the final release.

- These tarballs are committed by script against a Github repository of
Core. This will lose renames if changes are made, but this can't be
helped for now.

- Any patches or pull requests against the Github version are transformed
into e-mail patches and sent to JSC for incorporation into a future
version. A Git merge will naturally resolve these.

It's not an ideal workflow as the external consumers lose all the version
history, but it would be a lot better than the current tarball/dev sync
issues.

An alternative, if JSC can keep the Bitbucket repo up-to-date more
frequently, is for us to setup a unidirectional Bitbucket->Github mirror.
In practical terms, there seems little advantage though, as the Bitbucket
repo also loses version history of changesets at present (I think).

Whatever happens, a Github repo would extremely useful to fit into other
project workflows such as http://github.com/xen-org/xen-api (where there
are autobuildbots that could work against forks and pre-release for
continuous build coverage tests).

-anil

[Markus Mottl]

On Mon, Oct 1, 2012 at 2:36 PM, Anil Madhavapeddy <an...@recoil.org> wrote:
> I had a quick chat with Yaron about this at OUD. It seems that the best
> way to export changes from the JSC dev trees is via tarballs (that lose
> history). However, if we do not need patches to go *bidirectionally*, then
> the Mercurial export infrastructure could be significantly simplified.

I was under the (wrong?) impression that the bidirectional issues had
been successfully resolved with the patch queue repository. I can
imagine that releasing the internal dev tree (Core part) via tarballs
is less work for Jane Street than pushing changes to the external dev
tree. OTOH, it would be great if we could at least see changes in the
patch queue, even if it's just one huge blob rather than more
fine-grained patches.

For example, I don't even know which ones of my patches (if any) have
made it into the internal tree. If JS placed a patch into the queue
such that it takes already existing, internally applied patches into
account, external developers' lives would be much easier. If you want
to do things quickly without too much fine-grained control, you'd just
have to apply the internally applied patches to the last release and
create a new patch in the queue from the diff to the latest dev
release.

> I propose something along these lines:
>
> - JSC releases tarballs that are similar to the current ones, but with
> preX releases so we can see some changes before they're committed to
> the final release.

This seems neatly addressed by the patch queue. E.g. we might have
the policy that "Jane Street patches come first in the queue" so that
external developers have to fix their patches to stay compatible with
pre-releases rather than the other way round.

> - These tarballs are committed by script against a Github repository of
> Core. This will lose renames if changes are made, but this can't be
> helped for now.
>
> - Any patches or pull requests against the Github version are transformed
> into e-mail patches and sent to JSC for incorporation into a future
> version. A Git merge will naturally resolve these.
>

> An alternative, if JSC can keep the Bitbucket repo up-to-date more
> frequently, is for us to setup a unidirectional Bitbucket->Github mirror.
> In practical terms, there seems little advantage though, as the Bitbucket
> repo also loses version history of changesets at present (I think).

This seems rather involved. I think the fewer tools, scripts,
repositories, etc., we need to support the workflow, the better.

Has there ever been any attempt at JS to use the patch queue
repository to publish updates? I don't see any problems with this
fairly simple approach, but your mileage may vary.

Regards,

[Anil Madhavapeddy]

On 1 Oct 2012, at 12:20, Markus Mottl <markus...@gmail.com> wrote:

> On Mon, Oct 1, 2012 at 2:36 PM, Anil Madhavapeddy <an...@recoil.org> wrote:
>> I had a quick chat with Yaron about this at OUD. It seems that the best
>> way to export changes from the JSC dev trees is via tarballs (that lose
>> history). However, if we do not need patches to go *bidirectionally*, then
>> the Mercurial export infrastructure could be significantly simplified.
>
> I was under the (wrong?) impression that the bidirectional issues had
> been successfully resolved with the patch queue repository. I can
> imagine that releasing the internal dev tree (Core part) via tarballs
> is less work for Jane Street than pushing changes to the external dev
> tree. OTOH, it would be great if we could at least see changes in the
> patch queue, even if it's just one huge blob rather than more
> fine-grained patches.
>
> For example, I don't even know which ones of my patches (if any) have
> made it into the internal tree. If JS placed a patch into the queue
> such that it takes already existing, internally applied patches into
> account, external developers' lives would be much easier. If you want
> to do things quickly without too much fine-grained control, you'd just
> have to apply the internally applied patches to the last release and
> create a new patch in the queue from the diff to the latest dev
> release.

I agree; if the patchqueue approach works, then it would be simpler than
tarball exports. The Github mirror could also be auto-synched as a read
only (perhaps rebased) tree from this.

-anil

[Yury Sulsky]

Hi Markus,

I think 108.08 will be the first version where the external tree is sync'd to our internal one. We have tarballs for 108.07, but they include portability fixes and changes for ocaml 4 compatibility that didn't make it into our internal release. So rather than export that merge point, the current plan is to wait until 108.08 and from then on maintain a patch queue between releases.

[Yury Sulsky]

Great, The tarballs are up here: https://ocaml.janestreet.com/ocaml-core/108.07.01

Thanks!
Yury

[Markus Mottl]

On Mon, Oct 1, 2012 at 4:57 PM, Yury Sulsky <yury....@gmail.com> wrote:
> I think 108.08 will be the first version where the external tree is sync'd
> to our internal one. We have tarballs for 108.07, but they include
> portability fixes and changes for ocaml 4 compatibility that didn't make it
> into our internal release. So rather than export that merge point, the
> current plan is to wait until 108.08 and from then on maintain a patch queue
> between releases.

Thanks, good to know that the development tree and patch queue are
still on the future release roadmap.

[Anil Madhavapeddy]

This should be available on OPAM now too, if you do 'opam update'.

-anil

[Yaron Minsky]

I can't seem to get the new async to build, even in the latest opam.
By default it picks the older async, and when I pin to the latest
version, I get:

en $ opam upgrade async
No solution has been found:
- Conflict between type_conv.108.00.02 and type_conv.108.07.01.
+ type_conv.108.07.01 <- fieldslib.108.07.01 <- core.108.07.01 <-
async_core.108.07.01 <- async.108.07.01
+ type_conv.108.00.02 <- ocaml-data-notation.0.0.9

Any thoughts on how to get this to work?

y

[Yury Sulsky]

It looks like ocaml-data-notation should be upgraded to work with a
newer version of type_conv. If you're not using oasis, can you just
remove it?

Yury

[Anil Madhavapeddy]

That's correct; almost all non-Core packages are broken by the
type_conv interface change. I have some patches queued up for them,
but haven't had a chance to submit them upstream yet. Some (such
as Dyntype) I've already fixed, but I haven't done OASIS yet.

For now, a decent workaround is to switch to a compiler alias to
install OASIS, and otherwise use the new type_conv in a separate
switch.

-anil

[Yaron Minsky]

Is it surprising that uninstalling oasis causes a rebuild of Async and Core?

en $ opam remove oasis
The following variables are set in your environment, you should better
unset it if you want OPAM to work correctly.
- CAML_LD_LIBRARY_PATH
Do you want to continue ? [Y/n]
The following actions will be performed:
- remove oasis.0.3.0
- recompile async_core.108.00.02
- recompile async_unix.108.00.02
- recompile cow.0.3.2
- recompile core_extended.108.00.02
- recompile core.108.00.02
- recompile async_extra.108.00.02
0 to install | 6 to reinstall | 0 to upgrade | 0 to downgrade | 1 to remove
Do you want to continue ? [Y/n]

[Anil Madhavapeddy]

Well, removing OASIS should remove the constraint that should cause Async
to upgrade again. However, the actual constraint is on ODN and not OASIS,
so I wonder if this is a transitive dependency bug...

If you uninstall ocaml-data-notation, does Core upgrade to 107.00.01?

-anil

[Sylvain Le Gall]

If you send the patch for ODN, you''ll have a new upstream tarball by
then end of the day.

2012/10/5 Anil Madhavapeddy <an...@recoil.org>:

[Yaron Minsky]

By the way, no love on installing async 108.07.01 on opam. Here's the
tail of the error:

. /Users/yminsky/.opam/4.00.0/bin/ocamldep.opt -I +camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/type_conv -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/re -I
/Users/yminsky/.opam/4.00.0/lib/ulex -pp 'camlp4orf -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4' -modules
syntax/json/extension.ml > syntax/json/extension.ml.depends
. /Users/yminsky/.opam/4.00.0/bin/ocamlc.opt -c -I +camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/type_conv -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/re -I
/Users/yminsky/.opam/4.00.0/lib/ulex -pp 'camlp4orf -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4' -I syntax/json -I syntax
-o syntax/json/json.cmo syntax/json/json.ml
. /Users/yminsky/.opam/4.00.0/bin/ocamlc.opt -c -I +camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/type_conv -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/re -I
/Users/yminsky/.opam/4.00.0/lib/ulex -pp 'camlp4orf -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4' -I syntax/json -I syntax
-o syntax/json/extension.cmo syntax/json/extension.ml
. + /Users/yminsky/.opam/4.00.0/bin/ocamlc.opt -c -I +camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/type_conv -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/dyntype -I
/Users/yminsky/.opam/4.00.0/lib/re -I
/Users/yminsky/.opam/4.00.0/lib/ulex -pp 'camlp4orf -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4 -I
/Users/yminsky/.opam/4.00.0/lib/ocaml/camlp4' -I syntax/json -I syntax
-o syntax/json/extension.cmo syntax/json/extension.ml
. File "syntax/json/extension.ml", line 371, characters 36-39:
. Error: This expression has type bool but an expression was expected of type
. Camlp4.PreCast.Syntax.Ast.ctyp = Camlp4.PreCast.Ast.ctyp
. Command exited with code 2.
* make: *** [all] Error 10
'opam upgrade' failed

Has anyone gotten this to succeed? Also, how did json get implemented
here? There's no json support in async as far as I know...

y

[Anil Madhavapeddy]

On Fri, Oct 05, 2012 at 07:55:04AM -0400, Yaron Minsky wrote:
> By the way, no love on installing async 108.07.01 on opam. Here's the
> tail of the error:
>

> . File "syntax/json/extension.ml", line 371, characters 36-39:
> . Error: This expression has type bool but an expression was expected of type
> . Camlp4.PreCast.Syntax.Ast.ctyp = Camlp4.PreCast.Ast.ctyp
> . Command exited with code 2.
> * make: *** [all] Error 10
> 'opam upgrade' failed
>
> Has anyone gotten this to succeed? Also, how did json get implemented
> here? There's no json support in async as far as I know...

This looks like it's trying to install COW, and not Core. I think that
your pinning of type_conv might have confused things a little. I'll upload
a new COW and send Sylvain the ODN patches shortly, but it would be good
to understand why the package constraints aren't working as expected at
present...

Do you have COW installed also?

-anil

--
Anil Madhavapeddy http://anil.recoil.org

[Yaron Minsky]

I do have COW installed. That makes sense. I'll try deleting it.

[Anil Madhavapeddy]

Sylvain,

Patch put in a bug report at https://forge.ocamlcore.org/tracker/index.php?func=detail&aid=1226&group_id=148&atid=674

Would you consider moving ODN and the other OASIS dependencies to Github, since the main OASIS repo is also up there now?

Let me know when you cut a release and I'll update the OPAM constraints.

-anil

[Sylvain Le Gall]

2012/10/6 Anil Madhavapeddy <an...@recoil.org>:

> Sylvain,
>
> Patch put in a bug report at https://forge.ocamlcore.org/tracker/index.php?func=detail&aid=1226&group_id=148&atid=674
>
> Would you consider moving ODN and the other OASIS dependencies to Github, since the main OASIS repo is also up there now?
>

Not yet, give me time to adapt. If there were a way to have a forge
plugin that create a link between github and forge.ocamlcore.org, I
would more easily consider it, but until then, I will keep 2nd level
project where they are.

> Let me know when you cut a release and I'll update the OPAM constraints.
>

Jobs done, but don't have access to 108.07.01, so I let you check that
it compiles fine...