Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build p4runtime protobufs #328

Merged
merged 45 commits into from
Nov 15, 2023
Merged

Build p4runtime protobufs #328

merged 45 commits into from
Nov 15, 2023

Conversation

ffoulkes
Copy link
Contributor

@ffoulkes ffoulkes commented Nov 5, 2023

A cmake project that compiles the google rpc and p4runtime protobufs, generating serialization and gRPC service code for C++, Python, and Go. The outputs are:

  • Three tarballs (one per language), which are installed in the share/stratum/protobufs folder.
  • A Python wheel and source distribution, which are installed in the share/stratum/p4runtime folder.

This is in response to multiple requests that we supply a Python package, and one recent request to provide a Go package.

@ffoulkes ffoulkes added the cmake Affects CMake build system label Nov 5, 2023
@ffoulkes ffoulkes added the medium effort Moderate effort required label Nov 5, 2023
@ffoulkes ffoulkes marked this pull request as ready for review November 5, 2023 16:46
@ffoulkes
Copy link
Contributor Author

ffoulkes commented Nov 7, 2023

I changed the mechanics of the build so the protobufs build is done on demand, instead of being part of the regular build.

To build just the protobufs:

cmake --build build --target protobufs

This keeps the change from affecting developers who don't have Go (and a couple of additional packages) installed on their system. This includes the GitHub runners, which is how I discovered the problem.

As you can see from the number of commits I made, this was a tricky problem to solve. 🙂

@ffoulkes
Copy link
Contributor Author

ffoulkes commented Nov 9, 2023

I've added a README with build instructions.

protobufs/py/setup.cfg Outdated Show resolved Hide resolved
protobufs/py/setup.cfg Outdated Show resolved Hide resolved
protobufs/py/setup.cfg Outdated Show resolved Hide resolved
Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Rename HOST_PROTOC to HOST_PROTOC_COMMAND.

- Revise C++ gRPC file generation to limit itself to
  p4runtime.proto. None of the other protobuf files define
  services.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Put all the cmake files in the same folder.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Modularize by package (google, p4runtime) instead of by language
  (cpp, go, python). Create one target per (package, language)
  combination, and replace the tricksy path-manipulation logic
  with discrete lists of files.

- Generate an empty __init__.py file in each Python directory,
  making the output Python modules.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Move the include() commands after project(), to address an
  error raised by the GitHub workflow.

- Add support for building a Python wheel. Currently under
  development. Controlled by the ENABLE_WHEEL option.

- Go compiles are failing in the GitHub workflow. Placed
  them under control of the ENABLE_GO option, so they can
  be disabled until the issue is solved.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
- The GO_ENABLED option was disabled when building through the
  external project in the main listfile, despite having been set
  to TRUE when the option was defined. On inspection, it turned
  out that ALL the options were FALSE. cmake apparently does not
  apply the defaults when the build is done as an external project.
  Changed them to ordinary cache variables, and voila! All is
  right with the world. Pfui.

- Modify the GitHub workflow to upload the protobuf tarballs as
  artifacts.

- Modify .gitignore to ignore `build` and `install` directories
  anywhere in the tree, not just the root.

- Rename the install directory from `proto` to `protobufs`.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Try fixing the problem by moving `working-directory` after the
  `with` stanza.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- The previous fix didn't work. Try changing the path.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Correct BYPRODUCT names in tarball custom targets.

- Improve method used to generate __init__.py files.

- Use COPY instead of INSTALL to copy collateral files to wheel
  directory.

- Install Python packages under share/stratum/python.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Set package version to 2023.11.0.

- Update install requirements to track stratum-deps component
  versions.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
@ffoulkes ffoulkes requested a review from 5abeel November 13, 2023 23:06
@ffoulkes ffoulkes added major effort Significant effort required and removed medium effort Moderate effort required labels Nov 13, 2023
Signed-off-by: Derek G Foster <derek.foster@intel.com>
@ffoulkes ffoulkes changed the title Compile protobufs for c++, python, and go Build p4runtime protobufs Nov 14, 2023
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
- Modify GitHub workflow to install just the Python dependencies
  needed to build the P4Runtime protobufs.

- Remove invalid 'fcntl' module from requirements.txt file.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
@ffoulkes
Copy link
Contributor Author

The more I think about it, the more I wonder whether this should be part of networking-recipe at all. It's really an adjunct to p4runtime.

The choices would seem to be:

  1. Add support for wheel and tarball generation to the existing build procedures on ipdk-io/p4runtime-dev (and possibly upstream them).
  2. Create a separate repository for the purpose.

In both cases, we would need to update ipu_sdk to build the wheel and install it in the RFS image.

We would also have to consider how and whether to incorporate the wheel into networking-recipe build.

Oy.

- Extract logic to find the host Protobuf compiler and plugins
  from StratumDependencies.cmake and move it to a new HostProtoc
  find-module. StratumDependencies is now strictly for the target
  system.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
Signed-off-by: Derek G Foster <derek.foster@intel.com>
requirements.txt Outdated
@@ -1,4 +1,6 @@
p4runtime==1.3.0
Jinja2>=3.0.0
fcntl
select
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select needs to be removed as per #345

Signed-off-by: Derek G Foster <derek.foster@intel.com>
@@ -0,0 +1,25 @@
[metadata]
name = p4runtime
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called p4runtime-dev or even ipdk-io-p4runtime-dev to differentiate from upstream p4runtime?

Copy link
Contributor Author

@ffoulkes ffoulkes Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're currently differentiating our variant by using a distinctly different versioning scheme (YYYY-MM-N).

I think we want to use the same name as the existing module, so people don't have to change their programs to import something different.

Which is not to say that injecting ipdk-io into a name to distinguish it is a bad idea. I may steal it at the next appropriate opportunity... 😉

Copy link
Collaborator

@5abeel 5abeel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts and DCO needs to be resolved. Rest LGTM

Copy link
Collaborator

@nupuruttarwar nupuruttarwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.github/workflows/pipeline.yml Show resolved Hide resolved
@ffoulkes ffoulkes requested a review from nupurjai November 15, 2023 22:56
Signed-off-by: Derek G Foster <derek.foster@intel.com>
@ffoulkes ffoulkes merged commit f39325c into main Nov 15, 2023
5 checks passed
@ffoulkes ffoulkes deleted the compile-protobufs branch November 15, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Affects CMake build system major effort Significant effort required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants