search

libp2p / go-libp2p

libp2p implementation in Go
MIT License
5.98k stars 1.05k forks source link

make Protobuf generation deterministic #2429

Closed marten-seemann closed 9 months ago

marten-seemann commented 1 year ago

go generate ./... run locally should produce the same Protobuf output as when run on CI.

sukunrt commented 1 year ago

Looking at the protobuf version doc and the diffs here 

-//     protoc        v3.21.12
+//     protoc        v4.23.3

It looks like your machine's protoc version changed between the runs. The latest protc version is v23.3(on homebrew) which generates files with v4.23.3.

Should we mention the protobuf compiler version to be used somewhere in the repo? I don't see it mentioned anywhere currently.

marten-seemann commented 1 year ago

I was hoping we could do something like this: https://github.com/quic-go/quic-go/blob/05db808f72b9183653cfce97d6c235108bb18ee6/mockgen.go#L5

By using go run github.com/golang/mock/mockgen the version defined in go.mod is used. No need to worry about versions, it's all automatic. And updates are handled by bumping the version in go.mod.

It seems like this would be more difficult to accomplish with the protobuf compiler, since protoc has to be installed separately.

marten-seemann commented 1 year ago

Maybe the IPDX team has some ideas here. @galargh and @laurentsenta, have you encountered this before? Is there a good solution for this problem?

galargh commented 1 year ago

Some assume the risk and remove the version information from the generated files - https://github.com/golang/protobuf/issues/1185

Off the top of my head, one could also do go generate in docker with pinned deps - e.g https://github.com/moby/moby/blob/master/hack/dockerfiles/generate-files.Dockerfile

marten-seemann commented 1 year ago

I'd like to avoid using Docker for code generation. It would be nice if go generate ./... just worked.

Some assume the risk and remove the version information from the generated files - https://github.com/golang/protobuf/issues/1185

This sounds reasonable. I assume we'd still commit the files with the version information, but remove it in CI before comparing?

galargh commented 1 year ago

This sounds reasonable. I assume we'd still commit the files with the version information, but remove it in CI before comparing?

That's an interesting idea! From what I've seen people generally strip the versions before commit but I think doing it for compare only is more clever. We could even do one check with stripped version and if it fails do another one without stripping so that the version diff shows up in the output.

I'm going to track the implementation here: https://github.com/pl-strflt/uci/issues/26

marten-seemann commented 9 months ago

Closing since @galargh fixed this issue by adjusting the GHA workflows.