Subsystem versioned

[rgrinberg]

Fix #909

This is still WIP because I should really be saving the syntax version rather than the Jbuild vs. Dune version in the Info files. But this PR is still useful for initial review.

One thing that I'm wondering about is we should still generate the per subsystem versions in syntax (1, 0). Seems like jbuilder will not be able to load .dune files generated by dune anyway because of that initial 2.0 language.

[ghost]

One thing that I'm wdering about is we should still generate the per subsystem versions in syntax (1, 0). Seems like jbuilder will not be able to load .dune files generated by dune anyway because of that initial 2.0 language.

When the sub-system is defined in a jbuild file, dune should generate dune 1 files. Otherwise it's not going to work for the released versions of ppx_inline_test, ppx_expect, ppxlib, etc...

[ghost]

Actually I see that this is what is implemented by this PR

[ghost]

You can return List.rev acc, the error will be caught by the match loop [] with ....

[ghost]

This is still WIP because I should really be saving the syntax version rather than the Jbuild vs. Dune version in the Info files.

That seems right.

BTW, thinking about this again, where we read:

(dune
 2
 ((inline_tests.backend
   X.Y

we could set the M.syntax value of the subsystem to version X.Y since this is what we do when we generate the file.

[ghost]

And we set it to (0, 0) when the file start with (dune 1 ...

[ghost]

s/Dune/lang/

[ghost]

We shouldn't reopen the file a second time here. We need to construct a lexer function that first return the three extracted token and then switch to the real lexer.

[rgrinberg]

I'm not really sure how to do it the way you say however. Seems like we'd need to call parser with an initial set of lexemes (the 3 lexemes that we already parsed).

What should be possible is just reading the file into a string and then constructing the lexer from_string a second time. That should cut down on the sys calls.

[ghost]

There are two ways of doing it:

first one:

let pending_tokens = ref ... in
let lexer lb =
  match !pending_tokens with
  | [] -> lexer lb
  | x :: l -> pending_tokens := l; x
in
...

second one, which implements all the logic in one function:

let state = ref 0 in
let lexer = ref !Sexp.Lexer.token in
let lexer lb =
  let token = !lexer lb in
  (match !state, token with
   | 0, Lparen -> state := 1
   | 1, Atom (A "dune") -> state := 2
   | 2, Atom (A "1") -> state := 3; lexer := Sexp.Lexer.jbuild_token
   | 2, Atom (A "2") -> state := 3;
   | 2, Atom (A s) -> error ()
   | 3, _ -> ()
   | _ -> error ());
  token
in

[rgrinberg]

I see. I went for the first approach. Did not it was allowed to reuse the same state between possibly different lexer functions like this.

[ghost]

Ok. After reading this again, it seemed to me that the second version was clearer as all the logic is more self-contained, so I rewrote it using the second version.

[rgrinberg]

Jérémie Dimino <notifications@github.com> writes:

When the sub-system is defined in a jbuild file, dune should generate `dune 1` files. Otherwise it's not going to work for the released versions of ppx_inline_test, ppx_expect, ppxlib, etc...

Agreed. The simplification would apply to subsystems in dene files.

[ghost]

BTW, should we just save the whole parsing context in libraries?

[rgrinberg]

Do you see other uses for it? I'd rather have Jbuild.Library.t abstract away the parsing related details kind of like Lib.Info.t abstracts away the source of libraries when making a Lib.t.

[rgrinberg]

we could set the M.syntax value of the subsystem to version X.Y since this is what we do when we generate the file.

Sorry, this might be obtuse but isn't this done already? The X.Y in your example is a sub system version.

[ghost]

Do you see other uses for it? I'd rather have Jbuild.Library.t abstract away the parsing related details kind of like Lib.Info.t abstracts away the source of libraries when making a Lib.t.

Maybe when we switch to dune-package, but not for now. Happy to leave it as it for now.

Sorry, this might be obtuse but isn't this done already? The X.Y in your example is a sub system version.

If I read the code correctly, currently we set only Stanza.syntax to either (0, 0) or (1, 0). I suggest that we additionally set M.syntax to X.Y when parsing (dune 2 files.

[rgrinberg]

I see, thanks for clarifiyng. Why only for dune 2 files however? We don't hide the syntax version when parsing other sexps that comes from jbuild files.

[ghost]

I see, thanks for clarifiyng. Why only for dune 2 files however? We don't hide the syntax version when parsing other sexps that comes from jbuild files.

Because in dune 1 files the version of sub-systems is set to 1.0, and M.syntax is Stanza.syntax for the inline tests and ppx driver subsystems, so we'd be setting Stanza.syntax to 1.0 instead of 0.0.

[rgrinberg]

@diml I tried doing this in my last commit. I admit that I'm not fully confident about my change however.

[ghost]

That looks fine, we just a comment in the code explaining why we do this

[rgrinberg]

@diml added a comment

[ghost]

Not sure we should be that strict. In practice, it's likely that we'll be increasing the version of the dune language often. In version 2 files, the exact version is saved in each sub-system, so we can accept any version >= 1.0 here.

[ghost]

Even though we are not going to release another version of jbuilder, we should write (0, _) here (it's slightly clearer IMO)

[rgrinberg]

How come you aren't passing the version in the parsing context?

[ghost]

It is passed in of_sexp

[rgrinberg]

@diml the last commit relax the version checks I had before.