Use the new compiler for building images#4407
Conversation
This ports our image building path to use the new compiler and handles all the necessary test changes, etc. The new compiler brings a lot of improvements: - Richer diagnostics and error reporting - Support for Editions 2024 features - Incremental compilation (although this is mostly for the LSP, which is already using the new compiler)
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
| fmt.Sprintf( | ||
| `%s:5:1:detected cyclic import while importing "a/a.proto"`, | ||
| filepath.FromSlash("testdata/cyclicimport/b/b.proto"), | ||
| ), |
There was a problem hiding this comment.
I don't love this diagnostic, and the fact that the diagnostics in general are unable to provide the more robust details in a single FileAnnotation... e.g. this is the full report shows a much clearer cycle: https://github.com/bufbuild/protocompile/blob/main/experimental/ir/testdata/imports/cycle.proto.yaml.stderr.txt
That being said, if the user is printing the output to text, then they'll get the full information. This is only on the individual FileAnnotation (which would be used for things like in-line github annotations, the json output for analysis, etc.) -- and would provide a clear place where the import cycle was detected (although more details, in this case in particular would be nice). Will noodle this one a little.
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
just a couple minor things, this is looking good!
| alreadySeen, | ||
| nonImportFilenames, | ||
| seen, | ||
| xslices.ToStructMap(paths), |
There was a problem hiding this comment.
nit: call this once outside the loop?
| } | ||
|
|
||
| if len(targetFileInfos) == 0 { | ||
| // If we had no no target files within the module after path filtering, this is an error. |
There was a problem hiding this comment.
| // If we had no no target files within the module after path filtering, this is an error. | |
| // If we had no target files within the module after path filtering, this is an error. |
Just since we're nearby.
| bufctl.ExitCodeFileAnnotation, | ||
| "", // no stdout | ||
| filepath.FromSlash(`testdata/protofileref/noworkspaceormodule/fail/import.proto:3:8:import "`)+`google/type/date.proto": file does not exist`, | ||
| fmt.Sprintf( |
There was a problem hiding this comment.
This (and all others) is likely a breaking change. I am not sure if this matters on stderr, I can't remember, but buf was designed to output errors in the standard file:line:column:message format, which is relied upon by many tools.
There was a problem hiding this comment.
Did a sweep, and I think it's mostly ALE -> stdout. That being said, this is still a valid concern and @stefanvanburen had a good suggestion, which I am fully supportive of:
- Add a new
error-format=pretty, which would provide the new error report format - Detect if the output is TTY, which we already do for logging, and if it is, then we default to
error-format=pretty, and other wise, keep the default aserror-format=text. This would keep CI's consistent, ALE, etc. - This would apply to anything that produces compiler diagnostics and file annotations, so I can do a bit of work to structure this for
lint, breaking
Let me know if this approach seems sound to you!
There was a problem hiding this comment.
Discussed this offline, putting some notes here:
- No need for a new format name, instead we'll just default to this report format for TTY (and keep the old format for non-TTY)
- No changes to
lint, breaking'sstdoutoutput -- this will only be used for stderr compiler output
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
two minor nits. also, do we want a CHANGELOG entry? or are you going to add that as a part of the release?
private/buf/bufctl/controller.go
Outdated
| } else { | ||
| // When writing to stderr, check if the input is TTY, if so, allow printing the | ||
| // diagnostic report. | ||
| file, ok := c.container.Stdin().(*os.File) |
There was a problem hiding this comment.
nit: should be stderr to match the comment?
| file, ok := c.container.Stdin().(*os.File) | |
| file, ok := c.container.Stderr().(*os.File) |
There was a problem hiding this comment.
Leaving a note here before updating: chatted about this -- there is a limitation with app-go's wrapping of os.Stderr where it only implements io.Writer, and not os.File. We're going to put a well-documented workaround and follow-up after.
| got[i] = annotation.String() | ||
| } | ||
| require.Equal(t, len(want), len(got)) | ||
| assert.Equal(t, len(want), len(got)) |
There was a problem hiding this comment.
I think testifylint would flag this (we will enable someday!), but I think this should go back to require, because we will panic below if len(got) < len(want) when trying to access.
| assert.Equal(t, len(want), len(got)) | |
| require.Equal(t, len(want), len(got)) |
4 participants
This ports our image building path to use the new compiler and handles all the necessary test changes, etc.
The new compiler brings a lot of improvements:
Fixes #4193