deps: remove deprecated gogo proto #206
Conversation
|
Still need to update the bytes for the windows side of the tests but I need to do that on a windows machine. Is there a way we can improve the tests so it isn't reliant on the exact byte pattern for marshal? |
|
I attempted to run the tests on Windows to get the new hashes but I seem to have had almost every test fail due to permission issues there. The CI is reporting a success there but there seems to be some test failures there. |
jsternberg
force-pushed
the
gogoproto-remove
branch
3 times, most recently
from
2030300
to
747a7ee
Compare
|
Looks like the hashes didn't change when testing with the CI so going to undo that part of the change. This is ready for review now. |
jsternberg
force-pushed
the
gogoproto-remove
branch
from
747a7ee
to
ab6df96
Compare
docker-bake.hcl
Outdated
| @@ -39,6 +39,14 @@ target "lint" { | |||
| } | |||
| } | |||
|
|
|||
| target "generate" { | |||
There was a problem hiding this comment.
we should have another target validating generation similar to buildkit one:
- https://github.com/moby/buildkit/blob/e15601a00fbef2805db1ed87be7bb88628ae926b/hack/dockerfiles/generated-files.Dockerfile#L42-L56
- https://github.com/moby/buildkit/blob/e15601a00fbef2805db1ed87be7bb88628ae926b/docker-bake.hcl#L172-L177
and add it to the validate job matrix:
fsutil/.github/workflows/ci.yml
Lines 23 to 27 in 43b9329
There was a problem hiding this comment.
Added this. I also redid part of the generate script because running protoc is very frustrating. We want the import path to be the fully qualified path so that buildkit can import wire.proto using the fully qualified path, but protoc was just not happy with the paths being different between projects so I just decided "we're doing this in a dockerfile anyway so I'll just write the protoc command there rather than go generate."
jsternberg
force-pushed
the
gogoproto-remove
branch
2 times, most recently
from
15b2032
to
5822a71
Compare
docker-bake.hcl
Outdated
| @@ -39,6 +39,20 @@ target "lint" { | |||
| } | |||
| } | |||
|
|
|||
| target "validate-generate" { | |||
|
PTAL @kzys |
jsternberg
force-pushed
the
gogoproto-remove
branch
2 times, most recently
from
04798cd
to
87977d6
Compare
hack/update-generated-files
Outdated
| : ${CONTINUOUS_INTEGRATION=} | ||
|
|
||
| progressFlag="" | ||
| if [ "$CONTINUOUS_INTEGRATION" == "true" ]; then progressFlag="--progress=plain"; fi |
There was a problem hiding this comment.
This is not needed as there is no tty allocated on GHA and will therefore use plain progress. I think you can just remove this script
hack/validate-generated-files
Outdated
There was a problem hiding this comment.
Not sure why we need this script, could we not just have it inlined in the dockerfile like https://github.com/moby/buildkit/blob/23a2b6fab3f654e67e4407d5f9ef496ced12f029/hack/dockerfiles/generated-files.Dockerfile#L42-L56?
There was a problem hiding this comment.
I just copied what I saw the existing update and validate scripts were doing. We can always do it a different way.
There was a problem hiding this comment.
I've updated all of the hack scripts and the dockerfiles to use this inline technique rather than put too much logic in the hack scripts. The hack scripts now just invoke the appropriate bake target and the validate is now inlined in the dockerfile for gomod and generated-files.
This removes the deprecated gogo proto in favor of the standard implementation with the vtproto implementation for faster marshaling. The standard implementation still uses reflection and is slower than gogo, but gogo is deprecated due to the complexity of the code generation. vtproto is a project that does a similar thing to gogo but generates code in addition to the standard code generation rather than replacing it. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
jsternberg
force-pushed
the
gogoproto-remove
branch
from
87977d6
to
4879a07
Compare
This removes the deprecated gogo proto in favor of the standard implementation with the vtproto implementation for faster marshaling. The standard implementation still uses reflection and is slower than gogo, but gogo is deprecated due to the complexity of the code generation.
vtproto is a project that does a similar thing to gogo but generates code in addition to the standard code generation rather than replacing it.