Add package name to crit protobuf files

[snprajwal]

In Podman, the core.proto and sa.proto files from https://github.com/letsencrypt/boulder conflict with identically named proto files in crit, causing a panic during the build. As a workaround, these two files have been renamed to criu-core.proto and criu-sa.proto.

[adrianreber]

Can you describe the problem a bit more in detail.

[codecov-commenter]

Codecov Report

Merging #90 (4ca4d9a) into master (287ed08) will not change coverage.
The diff coverage is n/a.

❗ Current head 4ca4d9a differs from pull request most recent head def02ab. Consider uploading reports for the commit def02ab to get more accurate results

@@           Coverage Diff           @@
##           master      #90   +/-   ##
=======================================
  Coverage   51.38%   51.38%           
=======================================
  Files           2        2           
  Lines         288      288           
=======================================
  Hits          148      148           
  Misses        105      105           
  Partials       35       35           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rst0git]

Can you describe the problem a bit more in detail.

This pull request adds a fix for the problem that occurred in containers/podman#15591 after updating go-criu to v6.0.0:

panic: proto: file "sa.proto" is already registered
	previously from: "github.com/letsencrypt/boulder/sa/proto"
	currently from:  "github.com/checkpoint-restore/go-criu/v6/crit/images"
See https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

[rst0git]

LGTM

[adrianreber]

Not really convinced about this. This global protobuf namespace is very unfortunate. Can we fix it in another way?

In the link there is something about a go_package option. Can that be passed during proto -> pb.go compilation?

[snprajwal]

In the link there is something about a go_package option. Can that be passed during proto -> pb.go compilation?

@adrianreber According to the language guide at https://developers.google.com/protocol-buffers/docs/proto#packages:

• In Go, the package directive is ignored, and the generated .pb.go file is in the package named after the corresponding go_proto_library rule.

The go_package option is used to indicate the import path the the protoc compiler for .proto files with imports. This can be passed during compile time as --go-opt=Mpath/to/proto=importpath. We are doing this with the proto_opts variable in the Makefile here.

[adrianreber]

I also looked at it and found no real solution. The thing about this change is that it seems temporary until we meet another namespace collision. So maybe we have to namespace all files now. Would something like this work:

$ for i in criu-temp/images/*.proto; do if [[ "$i" == *"opts.proto" ]]; then continue; fi; sed -e 's,import\s",import "criu-,g' -i $i; done
$ for i in criu-temp/images/*.proto; do cp -v $i ./images/criu-$(basename $i); done

This is really unfortunate. Maybe we need to do this change on the CRIU level. It should not break the content of the existing images, I hope, but if we include the namespacing already in CRIU the changes here are no longer necessary.

[snprajwal]

@adrianreber @rst0git The issue is fixed in Go by adding a package directive to all the .proto files. This prevents conflicts in the protobuf namespace, while keeping the Go bindings unchanged. To ensure we don't have any more conflicts in the future, it would be prudent to pick a unique and verbose package name. I would suggest package checkpoint.restore (I tested with this, podman is building successfully).

Unfortunately, this change on criu breaks the generated C bindings. The package name is prepended to all protobuf messages, eg. checkpoint__restore__core_entry in core.pb-c.c, which generates a considerable number of errors. It would require a major overhaul of every reference to the messages in the codebase, which doesn't look like a practical refactor to me. Instead, we can keep the package directive exclusive to the Go bindings only, possibly by injecting it through the gen-proto target in crit/Makefile with something like:

ls crit/images/*.proto | xargs sed -i '/syntax/a package checkpoint.restore;\n'

[adrianreber]

If that works. That sounds much cleaner to me.

[adrianreber]

Looking at the change now, especially with a possible integration of this change in the main CRIU repository, I think criu as package name would make more sense than checkpoint.restore as all files are from the CRIU repository/project.

[snprajwal]

@adrianreber that was indeed my first thought too, but we cannot use package criu as it conflicts with the criu field in opts.proto.

[adrianreber]

@adrianreber that was indeed my first thought too, but we cannot use package criu as it conflicts with the criu field in opts.proto.

What is the error message you get?

[snprajwal]

What is the error message you get?

Something along the lines of criu has already been declared. Oddly enough, I tried it again now, and it works 🤔 will change and push

[adrianreber]

What is the error message you get?

Something along the lines of criu has already been declared. Oddly enough, I tried it again now, and it works thinking will change and push

😄 I also tried it and it was working, that's why I asked. Good to see that it works now.

[adrianreber]

This PR works for me. @rst0git what do you think.

At some point we can try to push this change to CRIU and remove the sed line here.

[rst0git]

LGTM