Fixes: Env var & delimiters, unversionned opam file pin, doc

[rjbou]

This PR closes #3391.

• It fixes OpamStd.split_delim implementation which had the same behaviour than OpamStd.split. Now they respectively keep & clean empty strings from the returned list.
• An option is added to OpamStd.Sys.split_path_variable, permiiting to retrieve a cleaned path or no.
• A fix on environment variables construction, in order to keep the trailing delimiter when it should be.

edit:

• Fix Minimal opam file incompatibility on v1.2 and v2 when pinning #3403: on local pin, versionned directory, if opam file is not versionned retrieve the one found locally in source directory
• Fix # 3398: Escape json extended ascii characters
• Update pinning doc

[dbuenzli]

You can't do this here, this will break UTF-8 output. The module assumes strings are UTF-8 encoded.

Btw. could you please make one PR per issue you fix.

[AltGr]

Awesome, thanks! I'll trust you have checked the many corner-cases :)

[AltGr]

I am wondering if the case when both are defined and different (Some local, Some vers when local <> vers; or maybe when not (OpamFile.OPAM.effectively_equal local vers)) would not deserve a warning too: "not using uncommitted version of opam file xxx".

But there is already the warning about uncommitted changes, so that might be redundant.

[dbuenzli]

I really think this "fix" muddies again the semantis of git pins and should not be merged. I don't see a clear ux benefit in doing so.

[rjbou]

@AltGr It would be complementary: on update, uncommitted changes warning is shown, in pinning, uncommitted opam changes warning is shown.

[dbuenzli]

Also on #3403 I wonder if there's really anything to fix. For me it works as expected, a git pin considers only what is in git.

[AltGr]

@dbuenzli there was still a consistency problem, because then we shouldn't use the non-versionned opam files for package detection (which would actually be more involved). It was a bit weird to detect the package definition file for pinning, but then say that no definition was found.

[dbuenzli]

@dbuenzli there was still a consistency problem, because then we shouldn't use the non-versionned opam files for package detection (which would actually be more involved).

Then why do you use the non-versionned opam file at all ? Just say no opam file was found, only consider what is in the VCS.

With this the system becomes idiosyncratic to understand. Now if I have an opam VCS file with uncommited changes why won't opam take it into account while it did take into account things that were not in the VCS before ? Keep these semantics simple.

[dbuenzli]

Then why do you use the non-versionned opam file at all ? Just say no opam file was found, only consider what is in the VCS.

See also #3408

I really think the way this is done too complicated and should be carefully reviewed (better, documented so that one can see if one can explains the behaviour in simple words, which will inform the design).

I think the confusion by @bobbypriambodo in #3403 is a confusion due to a change in defaults (and it's right to be confused by a change in defaults), I don't think it's a confusion in expectations.

[bobbypriam]

I think the confusion by @bobbypriambodo in #3403 is a confusion due to a change in defaults

I confirm this, it did throw me off because I remembered opam v1.2 worked that way. However I do agree with @dbuenzli that the behavior is consistent and reasonable. Perhaps it only warrants a documentation and/or a warning if opam cannot find an opam file? Something like no package definition is found -- perhaps you forgot to commit the opam file? (definitely could be better).

[rjbou]

I really think the way this is done too complicated and should be carefully reviewed (better, documented so that one can see if one can explains the behaviour in simple words, which will inform the design).

Indeed, it should be added in the new documentation. For the UX part, user is alerted by messages when uncommitted changes are discarded on upgrade, when an unversioned opam file is taken on pinning, when uncommitted opam file changes are discarded, etc.