Fix bufimageutil issues regarding weak imports #4322
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| // Keep 2: comment on package | ||
| package foo.bar; | ||
| // Keep if NestedFoo: comment on import any.proto | ||
| import "google/protobuf/any.proto"; |
There was a problem hiding this comment.
Annoyingly, this should be import weak. The actual filtered descriptor and source code info is correct. The issue turns out to be the way we package up the descriptor protos into protoreflect.FileDescriptor instances: those types no longer have any support for weak imports as of v1.36.5. 🤷
There was a problem hiding this comment.
Yeah, noticed that when we did the upgrade here :< #3653
| for i, indexFrom := range weakDependencies { | ||
| weakFrom := int32(i) | ||
| path := append(weakDependencyPath, weakFrom) | ||
| indexTo := dependencyChanges[indexFrom] | ||
| if indexTo == -1 { | ||
| sourcePathsRemap.markDeleted(path) | ||
| } else { | ||
| if indexTo != indexFrom { | ||
| sourcePathsRemap.markMoved(path, indexTo) | ||
| weakTo := int32(len(newWeakDependencies)) | ||
| if weakTo != weakFrom { | ||
| sourcePathsRemap.markMoved(path, weakTo) |
There was a problem hiding this comment.
This is the actual fix. Other stuff in this PR is incidental changes/improvement (trying to leave everything in better shape than when I arrived) and the updated test outputs.
| // TODO: Add support for option dependencies when buf CLI supports edition 2024. | ||
| if len(fileDescriptor.GetOptionDependency()) > 0 { | ||
| return nil, nil, nil, false, errors.New("edition 2024 not yet supported: cannot filter a file descriptor with option dependencies") | ||
| } |
There was a problem hiding this comment.
This is mostly a note for myself, I appreciate the TODO, I'm porting over the new compiler for edition 2024 support right now, so this is helpful for me when I rebase on this. Thank you!
2 participants
I was porting a lot of this code to a new stand-alone package (for now, in the bufbuild/extra repo) and when I was comparing test output in those ported tests to the original golden files in this package, there were some discrepancies. I was able to understand all of the discrepancies except ones related to weak imports in the "testdata/sourcecodeinfo" files. It turns out that there was a bug in the logic, that happened to manifest slightly differently between this original code and the ported code in extra.
So this fixes the logic. The issue is that it was using the "from" and "to" indexes in the
dependenciesfield ofFileDescriptorProtowhen updating source code info for theweak_dependenciesfield 😞. So this fixes that.I also updated the "testdata/sourcecodeinfo" golden files to include the actual filtered sources, instead of only the source code info blob, in order to compare the filtered source code info to those filtered sources when debugging. I updated a handful of other small things, too, like fixing stale symbols in doc comments.