Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce opam-version as first non-comment line for > 2.0 #4394

Closed
2 tasks done
dra27 opened this issue Oct 16, 2020 · 5 comments · Fixed by #4639
Closed
2 tasks done

Enforce opam-version as first non-comment line for > 2.0 #4394

dra27 opened this issue Oct 16, 2020 · 5 comments · Fixed by #4639
Assignees
Milestone

Comments

@dra27
Copy link
Member

dra27 commented Oct 16, 2020

At present, opam-version is used to control the fields available in an opam file but it can't control the lexer. In particular, this will be limiting for adding additional operators to the constraint language.

  • opam-file-format / opam 2.1 need updating to reject an opam file with opam-version: "2.1" or greater where opam-version is not the first line, ignoring lines consisting of just whitespace/comments.
  • opam 2.0.8 should cease parsing opam files after opam-version is encountered (so what a 2.0 lexer would regard as syntax errors are not reported)
@dra27 dra27 added this to the 2.0.8 milestone Oct 16, 2020
@dra27 dra27 self-assigned this Feb 5, 2021
@rjbou rjbou modified the milestones: 2.0.8, 2.0.9 Feb 5, 2021
@kit-ty-kate
Copy link
Member

Note that dune subst currently would not pass this check. For example:

$ cat dune-project
(lang dune 2.8)
(name test)
$ cat test.opam
opam-version: "2.0"
$ git add dune-project test.opam && git commit -m 'test'
[...]
$ dune subst
$ git diff
diff --git a/dune-project b/dune-project
index 1db5959..2ea4af5 100644
--- a/dune-project
+++ b/dune-project
@@ -1,2 +1,3 @@
 (lang dune 2.8)
 (name test)
+(version e5e5aba)
diff --git a/test.opam b/test.opam
index 013b84d..491ff2b 100644
--- a/test.opam
+++ b/test.opam
@@ -1 +1,2 @@
+version: "e5e5aba"
 opam-version: "2.0"

@dra27
Copy link
Member Author

dra27 commented Feb 8, 2021

Yes it would - it’s writing opam-version: "2.0" which is not affected by this change

@kit-ty-kate
Copy link
Member

Oh sorry I missed the part with opam-version: "2.1". All good then, sorry for the noise.

@dra27
Copy link
Member Author

dra27 commented Mar 11, 2021

This is addressed in ocaml/opam-file-format#43 and would be picked up by opam when the src_ext is updated.

Before you get errors like:

dra@thor:~/opam$ opam pin add opam-core .
[opam-core.2.0.8] synchronised (file:///home/dra/opam)
[ERROR] Invalid opam file in opam-core source from file:///home/dra/opam:
    error  2: File format error at line 7, column 0: '+' is not a valid token

now you get more informative errors like:

[ERROR] Invalid opam file in opam-core source from file:///home/dra/opam:
    error  2: File format error: unsupported or missing file format version; should be 2.0 or older

@dra27
Copy link
Member Author

dra27 commented Mar 11, 2021

The error message for opam-version in the wrong place is not as nice, but this can be improved in future (I've shied away from it here because it would require changing the OpamParser API again):

[ERROR] Invalid opam file in opam-core source from file:///home/dra/opam:
    error  2: File format error at line 4, column 0: Parse error

but it does clearly point to the line with the error, which is at least no worse than any other parsing error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants