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

ocamlbuild, improve _tags parsing and error report #6087

Closed
vicuna opened this issue Jul 26, 2013 · 10 comments
Closed

ocamlbuild, improve _tags parsing and error report #6087

vicuna opened this issue Jul 26, 2013 · 10 comments

Comments

@vicuna
Copy link

vicuna commented Jul 26, 2013

Original bug ID: 6087
Reporter: @dbuenzli
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2016-12-07T10:34:32Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.00.0
Target version: 4.02.0+dev
Category: -for ocamlbuild use https://github.com/ocaml/ocamlbuild/issues
Tags: junior_job
Related to: #4598 #5212 #6518
Monitored by: meyer @gasche @dbuenzli

Bug description

Great to see there are people willing to improve ocamlbuild !

So here are a few things:

  1. The following _tags file works:

true : package(gg),
package(vg)

  1. But not the following one:

true :
package(gg), package(vg)

  1. Neither the following one:

true
: package(gg), package(vg)

Could we

a)
Either be more consistant in the \ treatement or even better, avoid it altogether (I don't know if it would render the grammar ambiguous)

b)
Have a gnu-style error messages (http://www.gnu.org/prep/standards/standards.html#Errors)
so that we can easily jump to the location of the error from our editors. This is what is currently reported:

Lexical analysis error: _tags: Bad key in configuration line at line 1 (from file: "_tags")

@vicuna
Copy link
Author

vicuna commented Jul 26, 2013

Comment author: @gasche

Thanks, this kind of bug report is indeed helpful to give us a concrete repro case to work on.

(a) should definitely be fixed (\ should not change the validity of the syntax unless it splits words)

I'm not sure about (b) GNU style error messages; I think we should rather follow the global convention of the OCaml compiler error messages (which means that if you run ocamlbuild from any place that knows how to parse those messages, such as Emacs "compile" mode, you'll get the location parsed as well). Enabling GNU style error messages instead of the current one OCaml uses is a distribution-wide issue that has been discussed in the past (I think having patches for this would be interesting, but it's not on my radar of urgent things to do right now).

@vicuna
Copy link
Author

vicuna commented Jul 26, 2013

Comment author: meyer

Regarding (b): Maybe we can provide a flag for GNU message format. It's additional maintainance cost but probably relaively minimal.

I'd do the same for the rest of the toolchain where it's possible, but not that that I don't like the current situation.

@vicuna
Copy link
Author

vicuna commented Jul 26, 2013

Comment author: @dbuenzli

(b) Having what the general toolchain does is fine with me and I'd rather avoid adding new flags to the tools for that kind of details.

@vicuna
Copy link
Author

vicuna commented Jul 26, 2013

Comment author: meyer

OK, we should then be fixing it, so it behaves like the rest of the system, along with the newline separator fix. Thanks for the report.

@vicuna
Copy link
Author

vicuna commented Jul 26, 2013

Comment author: meyer

Let's tentatively schedule the fix for the release, it might slip away, but at least better support for Emacs mode is important, and comes at minimal cost and risk.

The other interesing way of dealing it with newlines would be via identation, so the spaces in the begining of line would be significant. (my mistake I am reporting it at the moment)

@vicuna
Copy link
Author

vicuna commented Jul 29, 2013

Comment author: @dbuenzli

Note error also occurs when you have a space (0x20) at the end of a _tags file line. Could you also check that please.

hexdump -C _tags
00000000 74 72 75 65 3a 20 70 61 63 6b 61 67 65 28 67 67 |true: package(gg|
00000010 29 20 |) |
00000012
ocamlbuild test.native
Lexical analysis error: _tags: Bad values in configuration line at line 1 (from file: "_tags")

@vicuna
Copy link
Author

vicuna commented Aug 19, 2013

Comment author: @damiendoligez

Note for the future: the toolchain's error report format will have to be changed at some point because it gives: (filename, line, char1, char2) where char1 and char2 relative to the beginning of line, while emacs wants (filename, line1, char1, line2, char2) where char2 is relative to the beginning of line2. (see #4598)

@vicuna
Copy link
Author

vicuna commented Aug 19, 2013

Comment author: @dbuenzli

Since it has to change anyways why not switch GNU style error messages (which are detected by default by emacs's compilation mode) ?

@vicuna
Copy link
Author

vicuna commented Jul 24, 2014

Comment author: @damiendoligez

That's what I intend to do.

@vicuna
Copy link
Author

vicuna commented Aug 21, 2014

Comment author: @gasche

Regarding the _tags syntax: I went with (a) make escape-newline placement more consistent, because I don't want to try to guess whether a given blurb is a pattern or a tag (it depends on whether a ':' follows, which does not cope very well with the fact that those are currently implemented with no-readahead lexers).

Patch 4.02@15118 makes all three examples accepted by ocamlbuild. These are in fact the only cases that have changed (escaped newlines before and after the ':'), so there were no consistency improvements in the rest of the grammar. But I didn't see any other place where accepting escaped newline would be natural.

The computation of error positions in presence of escaped newlines was also improved.

I think we can consider this particular PR resolved. If it doesn't already exists, I will open a new PR regarding the error-message format (so that we don't lose track of this issue), and you should feel free to open new PRs for other inconsistencies in the ocamlbuild parsing behavior.

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

No branches or pull requests

2 participants