Hijack the %{?val_if_true:val_if_false}% syntax to support extending the variables of packages with + in their name

[kit-ty-kate]

Per our own documentation, '+' should not be a valid character for packages. However packages like conf-g++ or conf-c++ now exist in the wild. This solution is a compromise in hope for {name1+name2+name3:var} to be parsable again

Related to ocaml/opam-file-format#59

[kit-ty-kate]

Instead of this, which is not backward compatible with opam 2.0 and 2.1, I would suggest hijacking the "%{?pk+*g1:var:}%" (one or more + in a string expension, with ? at the beginning and : at the end) syntax and adding a lint check when this syntax is present, that the package is marked with at least available: opam-version >= "2.2.0~beta2" (it should only be a warning, not an error as I expect cases where @dra27 would like to use this syntax in Windows context where the package itself should be compatible with opam < 2.2 but the value should be only reachable with opam 2.2)

See the definition for the details:

opam/src/format/opamTypesBase.ml

Line 125 in 87a88a5

let filter_ident_of_string s =

With 2.0 and 2.1 the syntax will output the empty string because the variable <empty string> is undefined and thus will fall into the else-or-undefined clause (the : at the end) and output <empty string>.
In 2.2 we can hijack this and take the core of the true case and reparse it without making + a separator. The only thing that we loose is be able to express the name1+name2+name3:var form but this is only a syntactic sugar for name1:var & name2:var & name3:var anyway and the name1+name2+name3:enable syntactic sugar which we can also replace by a more explicit version if needed.
EDIT: we also can't use the ternary var?true:false-or-undef inside the new clause due to the parsing rule but this is the same thing (can be desugared)

[dra27]

The workaround syntax sounds good to me!

[dra27]

Looks good, thanks, apart from what I think is a missing colon in the to_string function? This is working for the conf-g++ adaptations I'd made (wohoo - the package is no longer called conf-mingw-w64-gplusplus-x86_64!)

For the opam 3.0 reminder, another possibility might be to start the function in OpamFormatUpgrade, but the TODO comment also works 🙂

The only thing that we loose is be able to express the name1+name2+name3:var form but this is only a syntactic sugar for name1:var & name2:var & name3:var anyway and the name1+name2+name3:enable syntactic sugar which we can also replace by a more explicit version if needed.
EDIT: we also can't use the ternary var?true:false-or-undef inside the new clause due to the parsing rule but this is the same thing (can be desugared)

I just want to check I'm properly understanding - I think we're not losing the pkg+pkg+var or ternary var?true:false-or-undef here, because they don't presently work if pkg contains a +? It's that we're "only" gaining the ability to retrieve variable values as a string?

[kit-ty-kate]

I just want to check I'm properly understanding - I think we're not losing the pkg+pkg+var or ternary var?true:false-or-undef here, because they don't presently work if pkg contains a +? It's that we're "only" gaining the ability to retrieve variable values as a string?

yes. What i meant was that we can't use those syntaxes inside the new one (e.g. ?pkg1+pkg2:var: or ?pkg:var?x:y:) because it would clash with the way + and ? are parsed in this new context. So yes, there is no loss of ability, this PR only adds a way to refer to variables of packages with + in their name.

[rjbou]

We also need to update the doc for that particular case.
Should we add a lint to warn that it shouldn't be acceptable to have ++ at the end of a package name (even if supported by this hack)? Users installing already present packages won't see it until they pin it, and it'll minimise new packages addition.

[kit-ty-kate]

Should we add a lint to warn that it shouldn't be acceptable to have ++ at the end of a package name (even if supported by this hack)?

I don't think so. This PR basically officializes that we do accept packages with pluses in their names.

Users installing already present packages won't see it until they pin it, and it'll minimise new packages addition.

I'm not sure to understand this.

[rjbou]

Should we add a lint to warn that it shouldn't be acceptable to have ++ at the end of a package name (even if supported by this hack)?

I don't think so. This PR basically officializes that we do accept packages with pluses in their names.

Having a closer look, I'm afraid that the current PR is not enough to have a full support of pkg++ packages, it needs parser change too: { pkg++:installed } and its variations is not accepted by the parser, it is only available via string interpolation, in the form "%{?pkg++:installed:}%".
Either we have a full support, and then it needs a parser change, or it is a workaround to handle existing ones and then we need to propagate that (lint, manual).
In the second case (workaround), the changes then need to be in a specific function that is called by interpolation functions, only for the transformation from a string (no need to print, as the more global string will be reprinted as is). Current changes are misleading as it affects also OpamFormat.filter_ident parser/printer that is used for { pkg:installed } form.

Users installing already present packages won't see it until they pin it, and it'll minimise new packages addition.

I'm not sure to understand this.

When a package is installed from repository, there is no display of lint warning/errors. So having a lint won't affect people installing conf-g++, but it'll at least warn people creating packages and opam repo maintainers about packages ending with ++.

[rjbou]

I've updated the PR, details are in commit messages. Roughly there is

• addition of a lint when a package variable in a string have its package name containing several +, warn to use the new syntax (a single + is indistinguishable from the name1+name2:var form)
• Some code transformation to access functions to permit the lint
• Split filter_ident_of_string to have the good ones used when needed (no hijack for not in string variables)
• Remove the to_string modification as it is not affected by the hijack

As usual, there is a lot of tiny commits to show/explain the diff with current PR, if accepted, i'll clean commits history in order to have it in a mergeable state.

[dra27]

LGTM, thanks! I also double-checked that the mingw-w64-shims (which is now in opam-repository) is still working with it.

[kit-ty-kate]

We're missing the addition of the new syntax to the documentation (with the appropriate warnings ofc). I'll add it and rebase the branch

[rjbou]

Another 2 hands PR ^^ Thanks!

[kit-ty-kate]

^^ thanks!