Opam file with preserved format more precise #4302

rjbou · 2020-08-05T18:05:35Z

Fixes #3993. To not merge before #4298
To take the same example, with opam admin add-constraint dune>=1.11 :

diff --git a/packages/hardcaml/hardcaml.v0.12.0/opam b/packages/hardcaml/hardcaml.v0.12.0/opam
index ca3c8171bf..87f513d55c 100644
--- a/packages/hardcaml/hardcaml.v0.12.0/opam
+++ b/packages/hardcaml/hardcaml.v0.12.0/opam
@@ -10,13 +10,13 @@ build: [
   ["dune" "build" "-p" name "-j" jobs]
 ]
 depends: [
-  "ocaml"            {>= "4.07.0"}
-  "base"             {>= "v0.12" & < "v0.13"}
-  "ppx_jane"         {>= "v0.12" & < "v0.13"}
-  "stdio"            {>= "v0.12" & < "v0.13"}
+  "ocaml" {>= "4.07.0"}
+  "base" {>= "v0.12" & < "v0.13"}
+  "ppx_jane" {>= "v0.12" & < "v0.13"}
+  "stdio" {>= "v0.12" & < "v0.13"}
   "topological_sort" {>= "v0.12" & < "v0.13"}
-  "dune"             {>= "1.5.1"}
-  "zarith"           {>= "1.5"}
+  "dune" {>= "1.11"}
+  "zarith" {>= "1.5"}
 ]
 synopsis: "RTL Hardware Design in OCaml"
 description: "
diff --git a/packages/lwt/lwt.4.2.1/opam b/packages/lwt/lwt.4.2.1/opam
index 6c4fb5e040..431af99ad7 100644
--- a/packages/lwt/lwt.4.2.1/opam
+++ b/packages/lwt/lwt.4.2.1/opam
@@ -20,12 +20,11 @@ dev-repo: "git+https://github.com/ocsigen/lwt.git"
 
 depends: [
   "cppo" {build & >= "1.1.0"}
-  "dune"
-  "mmap"  # mmap is needed as long as Lwt supports OCaml < 4.06.0.
+  "dune" {>= "1.11"}
+  "mmap"
   "ocaml" {>= "4.02.0"}
-  "result" # result is needed as long as Lwt supports OCaml 4.02.
-  "seq" # seq is needed as long as Lwt supports OCaml < 4.07.0.
-
+  "result"
+  "seq"
   "bisect_ppx" {dev & >= "1.3.0"}
   "ocamlfind" {dev & >= "1.7.3-1"}
 ]

to

diff --git a/packages/hardcaml/hardcaml.v0.12.0/opam b/packages/hardcaml/hardcaml.v0.12.0/opam
index ca3c8171bf..83cdbee304 100644
--- a/packages/hardcaml/hardcaml.v0.12.0/opam
+++ b/packages/hardcaml/hardcaml.v0.12.0/opam
@@ -15,7 +15,7 @@ depends: [
   "ppx_jane"         {>= "v0.12" & < "v0.13"}
   "stdio"            {>= "v0.12" & < "v0.13"}
   "topological_sort" {>= "v0.12" & < "v0.13"}
-  "dune"             {>= "1.5.1"}
+  "dune" {>= "1.11"}
   "zarith"           {>= "1.5"}
 ]
 synopsis: "RTL Hardware Design in OCaml"
diff --git a/packages/lwt/lwt.4.2.1/opam b/packages/lwt/lwt.4.2.1/opam
index 6c4fb5e040..91781964df 100644
--- a/packages/lwt/lwt.4.2.1/opam
+++ b/packages/lwt/lwt.4.2.1/opam
@@ -20,7 +20,7 @@ dev-repo: "git+https://github.com/ocsigen/lwt.git"
 
 depends: [
   "cppo" {build & >= "1.1.0"}
-  "dune"
+  "dune" {>= "1.11"}
   "mmap"  # mmap is needed as long as Lwt supports OCaml < 4.06.0.
   "ocaml" {>= "4.02.0"}
   "result" # result is needed as long as Lwt supports OCaml 4.02.

kit-ty-kate · 2020-10-18T04:02:19Z

I've used this branch to make ocaml/opam-repository#17428 and it worked perfectly aside from one weird bug:

For gremlin.0.1.1, opam admin add-constraint "conduit-lwt-unix<3.0.0" results in the author field being moved from line 3 to the end of the file and be renamed authors (with an s)

Aside from that it was almost perfect (a few spaces were removed for the line being modified but the rest did not move and it feels so good!

Thanks for this! 🎉

mseri · 2020-10-18T08:31:31Z

Very neat job indeed.

Just a question remakr: we used to split conflicts on multiple lines otherwise the constraints were not correctly interpreted. Is this still an issue? If so, the tool is not splitting the conflicts on different lines, so that will need to be addressed as well

mseri · 2020-10-19T07:49:57Z

Please ignore my comment, I tested it locally and the | in the conflicts filters seems to work perfectly fine.
I think I got confused with an old unrelated issue (most likely ocaml/opam-repository#15441).

rjbou · 2020-10-20T13:38:59Z

Thanks for the tests!
@kit-ty-kate For the authors, I think I know from where it comes, but need to check first. For the spaces removed, it's normal. There (almost) no way to determine how the person wrote it's opam file, if it is organized by columns or just one space. So it is the default padding. The only magic that I introduced, is for the beginning of line (and newline) padding: it is always the same as previous line one. Thinking about it, if it is only a change in the filter, maybe i can keep the same padding as before... Need to check, this piece of code was written a months ago :)

kit-ty-kate · 2020-10-20T14:23:58Z

I haven't tried but would that also erase potential comments put at the end of the line being updated?

rjbou · 2020-10-20T14:48:33Z

From what I remember, it should keep them...

kit-ty-kate · 2020-10-23T17:15:13Z

This PR seems to have an unexpected effect on dune-release. In ocaml/opam-repository#17481 I was using a dune-release compiled on a switch where opam has this PR merged in and the dune comment on the first line wasn't removed as it is per usual.

I would guess they need some kind of way in the API to say "remove this comment" or something like that now with this change. cc @NathanReb

kit-ty-kate · 2020-10-24T01:53:18Z

Btw changes some API bits used in dune-release. Here is the change I had to make to dune-release to get it to work with the new API: tarides/dune-release@master...kit-ty-kate:opam-2.1
Is there a way to get some kind of backward compatible API? I feel like it might be annoying down the road for users if it ends up having a split of incompatible API.

rjbou · 2020-10-26T14:11:30Z

Comments: As the new version is more precise, it doesn't remove comments as before. It is indeed possible, in the function with_preserved_format to add a remove all comments optional argument, but it won't be available from cli, only via api.

kit-ty-kate · 2020-10-26T14:15:48Z

Comments: As the new version is more precise, it doesn't remove comments as before. It is indeed possible, in the function with_preserved_format to add a remove all comments optional argument, but it won't be available from cli, only via api.

dune-release only needs the API so it would be perfect!

rjbou · 2020-10-26T14:18:54Z

Ok, i'll add it then. Not that it will also remove all authors comments, not only dune ones.

NathanReb · 2020-10-26T14:41:00Z

Following an offline discussion with @kit-ty-kate it seems that the ideal behaviour in dune-release would be to remove only the dune comment.

Any thoughts on this?

If we agree that's the case then it's probably better to let dune-release filter out that comment. That will preserve other comments and allow dune-release code to be compatible with both 2.1 and 2.0 without conditional builds based on the opam version.

NathanReb · 2020-10-26T14:44:48Z

Ah nevermind my comment, the API already changes in a breaking way! In that case then yeah indeed, being able to drop all comments seems fine, it will preserve dune-release's behaviour between opam 2.0 and 2.1. We'll have to maintain compat between the two versions anyway!

rjbou · 2020-10-26T15:21:42Z

Thinking about it, It is more consistent (imho) for dune-release to remove the the dune comment than for opam to remove all comment just for one targeted line.
Anyway, it could be useful to have another function that clear all comments (and not difficult to do).

NathanReb · 2020-10-26T15:43:27Z

If I understood this correclty, OpamFile.OPAM.write_with_preserved_format drops all comments and that's what dune-release currently uses. Offering a way to clear all comments in the new opam API would be nice as I think we should focus on preserving the current behaviour first. Does that sound reasonable to you?

If we want to change dune-release to include other comments we can take care of it later on and indeed this will probably require dune-release to strip the dune comment itself as I don't think it makes sense for opam to allow modifying comments in the OpamFile API nor to be aware of this particular comment.

rjbou · 2020-10-26T16:31:04Z

The origin of this PR is that write_with_preserved_format was not precise enough on what is preserved. (cf. example in issue #4302) . As it is more precise, trying to modify only the concerned lines instead of fields, it is already a behaviour change. Keeping the comment is a side effect of trying to have the minimal change when writing.

rjbou · 2020-10-26T16:34:49Z

And yes, if it's ok for dune-release to use another function to drop comments, I'd prefer than removing them from new format, or adding an option to preserved format function. @AltGr an opinion on that ?

rjbou · 2020-11-18T10:42:43Z

no comment function ping @NathanReb @kit-ty-kate

rjbou · 2020-12-09T18:02:41Z

In fact, I don't think that the authors issue should be fixed here. As the field in a legacy one (it should be authors according the manual), we shouldn't keep a non accurate field.
There is a fallback to transform author into authors, I guess to not block for such a non critical field.

…'t drop comments, etc.

rjbou · 2020-12-09T18:12:19Z

Good to go for me

rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Aug 5, 2020

rjbou force-pushed the fmtendpos branch from b2eca78 to d8c5f49 Compare October 12, 2020 16:20

rjbou added this to the 2.1.0~beta3 milestone Oct 12, 2020

kit-ty-kate mentioned this pull request Oct 23, 2020

[new release] ppx_getenv (2.1) ocaml/opam-repository#17481

Merged

rjbou mentioned this pull request Oct 26, 2020

Update opam-format to opam-file-format new type definition #4298

Merged

rjbou mentioned this pull request Nov 17, 2020

No comment ocaml/opam-file-format#32

Closed

rjbou added 2 commits December 9, 2020 19:03

file: opam file with_preserved_format preserves in fields also, don…

c2e32ff

…'t drop comments, etc.

update changes

daa5d82

rjbou force-pushed the fmtendpos branch from d8c5f49 to daa5d82 Compare December 9, 2020 18:04

rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 9, 2020

rjbou merged commit f85aae3 into ocaml:master Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opam file with preserved format more precise #4302

Opam file with preserved format more precise #4302

rjbou commented Aug 5, 2020

kit-ty-kate commented Oct 18, 2020

mseri commented Oct 18, 2020

mseri commented Oct 19, 2020

rjbou commented Oct 20, 2020

kit-ty-kate commented Oct 20, 2020

rjbou commented Oct 20, 2020 •

edited

Loading

kit-ty-kate commented Oct 23, 2020

kit-ty-kate commented Oct 24, 2020 •

edited

Loading

rjbou commented Oct 26, 2020

kit-ty-kate commented Oct 26, 2020

rjbou commented Oct 26, 2020

NathanReb commented Oct 26, 2020

NathanReb commented Oct 26, 2020

rjbou commented Oct 26, 2020 •

edited

Loading

NathanReb commented Oct 26, 2020

rjbou commented Oct 26, 2020

rjbou commented Oct 26, 2020

rjbou commented Nov 18, 2020

rjbou commented Dec 9, 2020

rjbou commented Dec 9, 2020

Opam file with preserved format more precise #4302

Opam file with preserved format more precise #4302

Conversation

rjbou commented Aug 5, 2020

kit-ty-kate commented Oct 18, 2020

mseri commented Oct 18, 2020

mseri commented Oct 19, 2020

rjbou commented Oct 20, 2020

kit-ty-kate commented Oct 20, 2020

rjbou commented Oct 20, 2020 • edited Loading

kit-ty-kate commented Oct 23, 2020

kit-ty-kate commented Oct 24, 2020 • edited Loading

rjbou commented Oct 26, 2020

kit-ty-kate commented Oct 26, 2020

rjbou commented Oct 26, 2020

NathanReb commented Oct 26, 2020

NathanReb commented Oct 26, 2020

rjbou commented Oct 26, 2020 • edited Loading

NathanReb commented Oct 26, 2020

rjbou commented Oct 26, 2020

rjbou commented Oct 26, 2020

rjbou commented Nov 18, 2020

rjbou commented Dec 9, 2020

rjbou commented Dec 9, 2020

rjbou commented Oct 20, 2020 •

edited

Loading

kit-ty-kate commented Oct 24, 2020 •

edited

Loading

rjbou commented Oct 26, 2020 •

edited

Loading