I might be misunderstanding something, but the plan was to remove the findlib vars only in dune projects. It should still be available in jbuild projects.

Ok! I'll fix that!

BTW, we are currently refactoring the S-expression parsing API so that we can write versioned parsers more conveniently. Currently it is painful to do. @NathanReb you should probably wait until we finish this work as it will be much easier to adapt this PR once the new API is in place.

@NathanReb if you rebase on the current master, you can now access the version of the syntax in the S-expression parser via Syntax.get_exn Jbuild.syntax. This will give you the version of the Dune language being parsed: < (1, 0) is for the jbuild language and >= (1, 0) is for the dune one. For instance you could store this information with the action during parsing with use it in Super_context to decide whether to accept or reject this syntax.

Cool! I'll look into it today, sry for the delay!

Ok so I started working on this.

There's a now a test case for the jbuild backward compatiblity, it just lacks the implementation :)

From what I understand you can extract the syntax at parsing time using Stanza.file_kind so I used that to attach it to String_with_vars value in a first attempt to drop findlib only in dune files. That didn't quite turn out the way I hoped so here are a few questions:

• I was expecting the current implementation to work but Syntax.get_exn raises from my Stanza.file_kind invocation when I run make test, what could be the reasons for this to happen?
• Should I be using Stanza.file_kind at all ? If not what Syntax.t should I call Syntax.get_exn with?
• Attaching it to String_with_vars isn't satisfactory at all, I'd like to do that with Action which appears to be what you're suggesting. What would you think would be the best way to do it? Would it be ok to make Action_intf.Ast.t a record?
• Attaching the syntax to Action would work here but I guess this piece of information can be useful elsewhere in the future as some features arise in dune but not in jbuild files. Is that correct?

Actually, I believe @rgrinberg added a way to access the syntax when expanding variable in #905

I was expecting the current implementation to work but Syntax.get_exn raises from my Stanza.file_kind invocation when I run make test, what could be the reasons for this to happen?

This is usually because a Of_sexp.parse is made with a Univ_map.empty as the argument.

As diml pointed out, I have a way to access the syntax of the variable at expansion time which you might find useful. Though I think that doing this checking at parsing time as you're trying to is also alright.

I had another look at this while I'm reading #905. We indeed need to keep more than just File_kind.t, otherwise we might have the same issue when we switch to Dune 2. So we should indeed use Syntax.get_exn Stanza.syntax.

@NathanReb, I was indeed thinking about keeping this information at the level of actions, but actually keeping it in String_with_vars.t seems better, since as you point out we use String_with_vars.t in other contexts than actions where we might want this information as well.

We can pass the syntax version stored in String_with_vars.t to the callback passed to String_with_vars.expand. This should avoid the need for Action.syntax which is a bit complicated. If we still need this information at the level of actions, then turning Action_intf.Ast.t into a record seems fine. We already use Loc.t * Action.Unexpanded.t everywhere we store an action, so using a record seems good.

I was expecting the current implementation to work but Syntax.get_exn raises from my Stanza.file_kind invocation when I run make test, what could be the reasons for this to happen?

What tests are failing? The version of the syntax is implicitly passed to s-expression parsers. The failure might be due to a place where we forgot to fill it in.

I don't think it actually runs any test, it just crashes with an internal error before that. Here's the error:

$ make test
./_build/default/bin/main.exe runtest --dev
Internal error, please report upstream including the contents of _build/log.
Description: 
("Syntax identifier is unset" (name dune))
Backtrace:
Raised at file "src/stdune/exn.ml", line 32, characters 5-10
Called from file "src/stdune/sexp.ml", line 124, characters 5-8
Called from file "src/stdune/sexp.ml", line 123, characters 19-30
Called from file "src/stdune/sexp.ml" (inlined), line 120, characters 19-30
Called from file "src/string_with_vars.ml", line 66, characters 2-321
Called from file "src/stdune/sexp.ml", line 200, characters 15-29
Called from file "src/ordered_set_lang.ml", line 184, characters 16-73
Called from file "list.ml", line 82, characters 20-23
Called from file "list.ml", line 82, characters 32-39
Called from file "list.ml", line 82, characters 32-39
Called from file "src/ordered_set_lang.ml", line 190, characters 14-33
Called from file "src/ordered_set_lang.ml", line 194, characters 12-52
Called from file "src/stdune/sexp.ml", line 124, characters 5-8
Called from file "src/stdune/sexp.ml", line 453, characters 25-39
Called from file "src/stdune/sexp.ml" (inlined), line 120, characters 19-30
Called from file "src/jbuild.ml", line 544, characters 4-639
Called from file "src/stdune/sexp.ml" (inlined), line 120, characters 19-30
Called from file "src/jbuild.ml", line 733, characters 6-1023
Called from file "src/stdune/sexp.ml", line 507, characters 23-55
Called from file "src/stdune/sexp.ml", line 246, characters 19-28
Called from file "src/stdune/sexp.ml" (inlined), line 226, characters 24-53
Called from file "src/stdune/sexp.ml", line 242, characters 4-232
Called from file "src/stdune/sexp.ml", line 123, characters 19-30
Called from file "src/stdune/sexp.ml", line 351, characters 17-31
Called from file "src/stdune/sexp.ml" (inlined), line 226, characters 24-53
Called from file "src/stdune/sexp.ml", line 360, characters 4-559
Called from file "src/stdune/sexp.ml", line 200, characters 15-29
Called from file "list.ml", line 82, characters 20-23
Called from file "list.ml", line 82, characters 32-39
Called from file "src/stdune/list.ml" (inlined), line 29, characters 29-39
Called from file "src/jbuild.ml", line 1409, characters 4-78
Called from file "src/jbuild.ml", line 1435, characters 8-77
Called from file "src/jbuild_load.ml", line 195, characters 6-69
Called from file "src/jbuild_load.ml", line 254, characters 12-74
Called from file "map.ml", line 315, characters 19-42
Called from file "map.ml", line 315, characters 26-41
Called from file "map.ml", line 315, characters 19-42
Called from file "src/jbuild_load.ml", line 264, characters 4-42
Called from file "src/main.ml", line 43, characters 4-70
Called from file "src/main.ml", line 288, characters 12-56
Called from file "bin/main.ml", line 767, characters 7-29
Called from file "vendor/cmdliner/src/cmdliner_term.ml", line 27, characters 19-24
Called from file "vendor/cmdliner/src/cmdliner.ml", line 106, characters 32-39
Called from file "vendor/cmdliner/src/cmdliner.ml", line 136, characters 18-36
Called from file "vendor/cmdliner/src/cmdliner.ml", line 251, characters 22-48
Called from file "bin/main.ml", line 1543, characters 10-51
Makefile:24: recipe for target 'test' failed
make: *** [test] Error 1

and here's the content of _build/log:

$ cat _build/log 
# ./_build/default/bin/main.exe runtest --dev
# OCAMLPARAM: unset

Ah, it's a case we found recently. I believe this one is fixed by #905

Ok, in the meantime I'll into how to get read of Action.Unexpanded.syntac and use an int * int rather than Stanza.File_kind!

#905 is now merged in master

I pushed some changes:

• the syntax version is attached to String_with_vars.t values at parsing time
• expand and partial_expand callback now takes a second Syntax.Version.t argument
• findlib is supported in jbuild

This is still not ready though because I need to set the syntax version for virt, virt_var and virt_text. Do you have any suggestion on how this should be handled?

Looking at how virt is currently used in the code, (0, 0) is the right choice.

I guess you can make just document that virt expect a string in jbuild syntax. The rest of the PR looks good.

You can probably extract a let virt_syntax = (0, 0) to document that (and remove the XXXs 😉 ). Don't forget the DCO (git commit -s) and I believe we're good to go.

Just made virt's syntax 0,0, updated the doc comment and added the signed off by commit comment!

Looks good, thanks!

I had another look at this while I'm reading #905. We indeed need to keep more than just File_kind.t, otherwise we might have the same issue when we switch to Dune 2. So we should indeed use Syntax.get_exn Stanza.syntax.

Having another look at the PR, I find this point less convincing now. The way I see Syntax.t conceptually is as a versioning mechanism for sexp parsers. But we know that the templating language is implemented at a lower level and isn't really subject to the same versioning scheme apart from the more crude jbuild vs dune distinction.

The entity that seems to be versioned here is the expansion functions themselves rather than the templates. In any case, I don't think it's worth to hold up the PR because of this. Since the variable expansion system needs a bit of a renovation to handle things such #928 better, so I approve once all the dune and jbuild files are correctly formatted.

@rgrinberg the versions that users write in their dune-project file should control alll the features that are available. From a usability point of view, adding, removing or renaming a variable is the same as adding, removing or renaming a stanza, so it seems natural to me that to use the version numbers for both.

Regarding the formatting, eventually we should have dune pp that would automatically format dune files like ocamlformat automatically format OCaml source files. Then we could have a target to reformat all the files, but in the meantime let's not be too fussy about the formatting inside blackbox tests.

(BTW, there seem to be an issue with the DCO bot, it's stuck again)

I agree that our versioning applies to adding or removing variables. It's
just that I see it as versioning the variables and not the templates. I.e.
if we have a single template %{foo}, we should be able to expand it with
different versions of the syntax. E.g. if we somehow load a subsystem from
a jbuild file but expand it in a dune rule, then we should use dune's
variable definitions.

Actually, I think in this case we should use jbuild's definitions. The reason for that is the the author of the subsystem controls the definition of the subsystem but doesn't know where the subsystem is going to be used. If we used dune's definition, the interpretation of variables might not be the same as the one where the subsystem was defined, which doesn't seem right.

👍 for dune fmt. I think I heard something about an autoformatter for s-expressions, surely Jane Street uses one internally?

Yh, we have one. I haven't finished preparing for release. The only problem is that it has a few dependencies. Additionally it is not aware of what is field, a constructor etc... in dune files.

OK, I'll open an issue so that we can discuss/coordinate over there.

Thanks @emillon

I also think that we should just roll our own formatter. We already pretty print sexps in many other places anyway.