Update P4Runtime and use FetchContent instead of a submodule.

[fruffy]

Use FetchContent to pull in P4Runtime and remove the submodule. While doing this we also update P4Runtime to the latest version.

Also clean up some of the DPDK paths, the generated Protobuf files were polluting the top-level build folder.

[fruffy]

@smolkaj I would like to make the include path of the generated Protobuf files consistent with the rest of the compiler includes, i.e.

#include "p4/config/v1/p4info.pb.h" -> #include "control-plane/p4/config/v1/p4info.pb.h"
#include "p4/config/dpdk/p4info.pb.h" -> #include "backends/dpdk/p4/config/p4info.pb.h"

I can do this in CMake, but I am unsure how to do this with Bazel. Is this possible? If not, I will just stick with the original style.

[smolkaj]

Very likely possible, but I need a bit more context.

In Bazel, include paths are by default relative to your WORKSPACE.bazel file.
Then there is all kind of magic to strip or add prefixes from/to the include path, including the following

• The import_prefix and strip_import_prefix attributes of the proto_library rule, see documentation.
• You can search for includes relative to arbitrary directories by setting the includes property of your build rules, see e.g. documentaton

EDIT: And then there is a well-know limitation related to this, reported by yours truly: grpc/grpc#33377

[fruffy]

Very likely possible, but I need a bit more context.

Well, essentially I would like to control the place the proto files are generated to. With that I should also be able to manipulate the include path. This seems possible for project-local declarations, for example here: https://github.com/p4lang/p4c/blob/main/BUILD.bazel#L283

However, I do not know how to do this for a dependency and its generated proto files, for example P4Runtime.

EDIT: And then there is a well-know limitation related to this, reported by yours truly: grpc/grpc#33377

Hmm, this might be a little bit of a blocker.

[smolkaj]

However, I do not know how to do this for a dependency and its generated proto files, for example P4Runtime.

P4Runtime does not generate .proto files at compile time. All .proto files are static, just like other source files (.h/.cc). And so the static path becomes your include path, unless you use some of the options I described above.

One think you could do is use a genrule that moves the .proto file from one place to another, which would be similar to the example you pointed to (https://github.com/p4lang/p4c/blob/main/BUILD.bazel#L283) but using cmd = "mv ...". This would be an okay workaround, but standard practice is to use fixed, static paths for source files such as .proto files.

[fruffy]

P4Runtime does not generate .proto files at compile time. All .proto files are static, just like other source files (.h/.cc). And so the static path becomes your include path, unless you use some of the options I described above.

At least with CMake we call protoc at some point and it serializes the files into the folder we specify. This lets us precisely control the output directory and with that we can also make sure that include path is consistent with the rest of the compiler includes. I am guessing Bazel, or the Bazel Protobuf recipe does that for you?

I reverted the control-plane changes, will give this another shot once I understand the Bazel build structure a little bit better.

[smolkaj]

Thanks for clarifying, I'm starting to understand better.

At least with CMake we call protoc at some point and it serializes the files into the folder we specify. This lets us precisely control the output directory and with that we can also make sure that include path is consistent with the rest of the compiler includes. I am guessing Bazel, or the Bazel Protobuf recipe does that for you?

Ah, okay...you are talking about the C++ API that is generated by protoc based on the .proto files?

In Bazel, the BUILD rule for generating the C++ APIs is cc_proto_library. indeed, the documentation says very little about how it works. The rule is very opinionated and doesn't allow you to control the output path. This is probably a case of Google conventions making its way into Bazel. The relevant conventions are:

• The BUILD rule for compiling a source file path/to/source/file.ext should live in path/to/source/BUILD.bazel.
• If the BUILD rule produces an output, the output will generally live at the same path as the source files(s) and/or build rule (but note that the output will not show up in your source tree -- it will only show up in the sandbox Bazel uses for the build)

Perhaps confusingly, while cc_proto_library doesn't provide any control for adjusting the path of the output files, the proto_library does, via the import_prefix and strip_import_prefix attributes I mentioned above. If you combine the two attributes, you should be able to generate the output files at a arbitrary path:

proto_library(
  ...,
  src = ["//path/relative/to/workspace/root/my_schema.proto"]
  import_prefix = "any/path/you/want/",
  strip_import_prefix = "/path/relative/to/workspace/root/",
)

Then you should be able to say

#include "any/path/you/want/my_schema.pb.h"

in your C++ files.

[jafingerhut]

LGTM, but to qualify my approval, I mainly care that we don't break the build, and keep things manageable for us. In general using FetchContent in cmake seems like a good direction to move. Any breakages in build I expect CI to catch, or my monthly builds.

[smolkaj]

@smolkaj I would like to make the include path of the generated Protobuf files consistent with the rest of the compiler includes, i.e.

#include "p4/config/v1/p4info.pb.h" -> #include "control-plane/p4/config/v1/p4info.pb.h"
#include "p4/config/dpdk/p4info.pb.h" -> #include "backends/dpdk/p4/config/p4info.pb.h"

After taking a closer look, it should be somewhat straightforward to achieve:

Currently:

        git_repository(
            name = "com_github_p4lang_p4runtime",
            ....
            # strip_prefix is broken; we use patch_cmds as a workaround,
            # see https://github.com/bazelbuild/bazel/issues/10062.
            # strip_prefix = "proto",
            patch_cmds = ["mv proto/* ."],

I think the following should work:

patch_cmds = [
  "mkdir -p 'control-plane'",
  "mv proto/* control-plane"
]

Then in BUILD.bazel, update e.g.

"@com_github_p4lang_p4runtime//:p4info_cc_proto",

to

"@com_github_p4lang_p4runtime//control-plane:p4info_cc_proto",

[fruffy]

I think the following should work:

patch_cmds = [
  "mkdir -p 'control-plane'",
  "mv proto/* control-plane"
]

Then in BUILD.bazel, update e.g.

"@com_github_p4lang_p4runtime//:p4info_cc_proto",

to

"@com_github_p4lang_p4runtime//control-plane:p4info_cc_proto",

Great, I tried the first step but could not figure out how to resolve the errors afterwards.

[fruffy]

Hmm, I am running into:

But now I am running into: ERROR: package contains errors: : //:ir_frontend_midend_control_plane: invalid label '@com_github_p4lang_p4runtime//:control-plane:p4info_cc_proto' in element 5 of attribute 'deps' in 'cc_library' rule: invalid target name 'control-plane:p4info_cc_proto': target names may not contain ':' 

Replaced the colon with a slash and got

ERROR: p4c/BUILD.bazel:181:11: no such target '@com_github_p4lang_p4runtim
e//:control-plane/p4info_cc_proto': target 'control-plane/p4info_cc_proto' not declared in package '' defined by 
.cache/bazel/_bazel_fruffy/f06add6260f0caee65f0101bb907f7e3/external/com_github_p4lang_p4runtime/BUI
LD.bazel (Tip: use `query "@com_github_p4lang_p4runtime//:*"` to see all the targets in that package) and referen
ced by '//:ir_frontend_midend_control_plane'

[smolkaj]

You have the colon in the wrong place:

@com_github_p4lang_p4runtime//:control-plane:p4info_cc_proto -> @com_github_p4lang_p4runtime//control-plane:p4info_cc_proto

EDIT: The syntax is `@<worskpace_name>//<path_to_build_file_relative_to_workspace_root>:<target_declared_in_build_file>

[fruffy]

You have the colon in the wrong place:

@com_github_p4lang_p4runtime//:control-plane:p4info_cc_proto -> @com_github_p4lang_p4runtime//control-plane:p4info_cc_proto

EDIT: The syntax is `@<worskpace_name>//<path_to_build_file_relative_to_workspace_root>:<target_declared_in_build_file>

I tried a bunch of things but I could not figure out the right syntax, maybe you can tell from the error what to do?

If not, it's not that important, I will just revert to the previous include behavior.

[smolkaj]

control-plane/p4/v1/p4runtime.proto:19:1: Import "p4/config/v1/p4info.proto" was not found or had errors.
control-plane/p4/v1/p4runtime.proto:20:1: Import "p4/v1/p4data.proto" was not found or had errors.

Since we are changing the location of some .proto files, the include paths within the .proto files themselves are now breaking...
Perhaps not worth going down this rabbit hole.

[fruffy]

control-plane/p4/v1/p4runtime.proto:19:1: Import "p4/config/v1/p4info.proto" was not found or had errors.
control-plane/p4/v1/p4runtime.proto:20:1: Import "p4/v1/p4data.proto" was not found or had errors.

Since we are changing the location of some .proto files, the include paths within the .proto files themselves are now breaking... Perhaps not worth going down this rabbit hole.

I will just revert and merge.