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

[wpiutil] Change C++ protobuf to nanopb #7309

Merged
merged 75 commits into from
Nov 8, 2024

Conversation

ThadHouse
Copy link
Member

No description provided.

@ThadHouse ThadHouse requested review from PeterJohnson and a team as code owners October 29, 2024 00:41
@calcmogul calcmogul changed the title Import nanopb [upstream_utils] Import nanopb Oct 29, 2024
@ThadHouse ThadHouse force-pushed the nanopb branch 2 times, most recently from 65d5b5a to 5024562 Compare October 29, 2024 00:55
Copy link
Contributor

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

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

By convention, the generated folder hierarchy mirrors the folder hierarchy in main, so the generated files should go under src/generated/main/native/cpp instead of src/generated/main/cpp.

wpimath/generate_nanopb.py Outdated Show resolved Hide resolved
@Gold856
Copy link
Contributor

Gold856 commented Nov 2, 2024

Here's a patch that should fix Bazel:

diff --git a/wpiutil/BUILD.bazel b/wpiutil/BUILD.bazel
index 0a97abe53..33760ee47 100644
--- a/wpiutil/BUILD.bazel
+++ b/wpiutil/BUILD.bazel
@@ -225,6 +225,14 @@ cc_library(
     visibility = ["//visibility:public"],
 )
 
+cc_library(
+    name = "nanopb-test-headers",
+    hdrs = glob(["src/generated/test/native/cpp/*.h"]),
+    includes = ["src/generated/test/native/cpp"],
+    strip_include_prefix = "src/generated/test/native/cpp",
+    visibility = ["//wpiutil:__subpackages__"],
+)
+
 cc_test(
     name = "wpiutil-cpp-test",
     size = "small",
@@ -237,6 +245,7 @@ cc_test(
     deps = [
         ":wpiutil.static",
         ":wpiutil-testlib",
+        ":nanopb-test-headers",
         "//thirdparty/googletest:googletest.static",
     ],
 )

@pjreiniger
Copy link
Contributor

I tested @Gold856 patch locally and he is correct

@PeterJohnson PeterJohnson changed the title [upstream_utils] Import nanopb [wpiutil] Change C++ protobuf to nanopb Nov 3, 2024
@ThadHouse
Copy link
Member Author

/format

@PeterJohnson PeterJohnson requested review from a team as code owners November 3, 2024 03:56
@ThadHouse
Copy link
Member Author

/format

remove java
}
return
}
Copy link
Member

Choose a reason for hiding this comment

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

How is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was needed because wpiutil only had protobufs in the test, and the google protobufs were erroring for some reason.

I’ll be removing all of this Infra in a future pr. This pr was just already so big it wasn’t worth it.

@PeterJohnson PeterJohnson merged commit 8b8b634 into wpilibsuite:main Nov 8, 2024
41 checks passed
@ThadHouse ThadHouse deleted the nanopb branch November 8, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants