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

allow json output format to be modified #4407

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

grg
Copy link
Contributor

@grg grg commented Feb 9, 2024

Expose JsonPrintOptions to allow JSON output format to be modified.

@grg grg force-pushed the grgibb/control_plane_json_options branch from 1aa930c to b87d3b6 Compare February 9, 2024 23:13
@grg grg marked this pull request as ready for review February 10, 2024 00:14
@grg
Copy link
Contributor Author

grg commented Feb 10, 2024

Many of the tests are failing because dependencies can't be fixed:

E: Failed to fetch http://download.opensuse.org/repositories/home:/p4lang/xUbuntu_20.04/amd64/p4lang-pi_0.1.0-15_amd64.deb  500  Internal Server Error [IP: 195.135.223.226 80]
Fetched 14.3 MB in 2s (7397 kB/s)
E: Failed to fetch http://download.opensuse.org/repositories/home:/p4lang/xUbuntu_20.04/amd64/p4lang-bmv2_1.15.0-7_amd64.deb  500  Internal Server Error [IP: 195.135.223.226 80]

What do we need to do to resolve this? Or is it just a case of wait for something on the source to update?

@fruffy
Copy link
Collaborator

fruffy commented Feb 10, 2024

Many of the tests are failing because dependencies can't be fixed:

E: Failed to fetch http://download.opensuse.org/repositories/home:/p4lang/xUbuntu_20.04/amd64/p4lang-pi_0.1.0-15_amd64.deb  500  Internal Server Error [IP: 195.135.223.226 80]
Fetched 14.3 MB in 2s (7397 kB/s)
E: Failed to fetch http://download.opensuse.org/repositories/home:/p4lang/xUbuntu_20.04/amd64/p4lang-bmv2_1.15.0-7_amd64.deb  500  Internal Server Error [IP: 195.135.223.226 80]

What do we need to do to resolve this? Or is it just a case of wait for something on the source to update?

@rst0git The opensuse repository is quite flaky and we run into this a lot in CI. Do you know a more stable repository we could migrate to? What is the process like?

@rst0git
Copy link
Member

rst0git commented Feb 10, 2024

Do you know a more stable repository we could migrate to? What is the process like?

Sure, we can use Launchpad instead. I'll update the packages in Launchpad and open a pull request for the installation process in CI.

@fruffy fruffy added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Feb 10, 2024
@fruffy
Copy link
Collaborator

fruffy commented Feb 10, 2024

Do you know a more stable repository we could migrate to? What is the process like?

Sure, we can use Launchpad instead. I'll update the packages in Launchpad and open a pull request for the installation process in CI.

Great, thank you!

@grg
Copy link
Contributor Author

grg commented Feb 12, 2024

The Mac build in test suite fails as follows:

In file included from /Users/runner/work/p4c/p4c/frontends/common/options.h:24:
/Users/runner/work/p4c/p4c/control-plane/p4RuntimeSerializer.h:23:10: fatal error: 'google/protobuf/util/json_util.h' file not found
#include <google/protobuf/util/json_util.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm going to change a class instance to a pointer to avoid this. But for my own education, how would I fix this if I wasn't going to switch to a pointer.

(The pointer allows me to just write class ClassINeedFromHeaderFile; instead of importing the whole header file.)

@grg grg force-pushed the grgibb/control_plane_json_options branch from b87d3b6 to 88e0467 Compare February 12, 2024 21:16
@fruffy
Copy link
Collaborator

fruffy commented Feb 12, 2024

I'm going to change a class instance to a pointer to avoid this. But for my own education, how would I fix this if I wasn't going to switch to a pointer.

Honestly, I am not sure why this one is failing. It could be a CLang vs GCC issue. I enabled the sanitizer, which uses CLang instead of GCC to check that.

@grg grg force-pushed the grgibb/control_plane_json_options branch from 88e0467 to f35d887 Compare February 12, 2024 21:39
@grg
Copy link
Contributor Author

grg commented Feb 12, 2024

I'm going to change a class instance to a pointer to avoid this. But for my own education, how would I fix this if I wasn't going to switch to a pointer.

Honestly, I am not sure why this one is failing. It could be a CLang vs GCC issue. I enabled the sanitizer, which uses CLang instead of GCC to check that.

I pushed a change that switches to a pointer instead. Don't entirely know if I like it (I need to create a JsonPrintOptions object in one of the constructors for P4Runtime). It's the most recent commit.

BTW, if you want to test my PR without this change, feel free to temporary back out the last commit.

@grg
Copy link
Contributor Author

grg commented Feb 12, 2024

I'm going to change a class instance to a pointer to avoid this. But for my own education, how would I fix this if I wasn't going to switch to a pointer.

Honestly, I am not sure why this one is failing. It could be a CLang vs GCC issue. I enabled the sanitizer, which uses CLang instead of GCC to check that.

Following up, I think that we need to add a target_include_directories call in cmake files for any cpp files that directly or indirectly need the Protobuf includes. Below is a sample from backends/p4tools/common/CMakeLists.txt. I'm guessing that the Mac OS protobuf directory is outside of the "default" include directories for the build, whereas the Linux location is.

target_include_directories(
  p4tools-control-plane
  SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS}
)

I'd prefer not to have to extend the set of sources that require the protobuf includes, unless we chose to include it for some large chunk of files (e.g., entire build, all frontend files, ...). The failure above came about by including options.h: I want to avoid the scenario where each time someone includes options.h they also need to go and update the list of files that need the protobuf includes.

@fruffy
Copy link
Collaborator

fruffy commented Feb 13, 2024

What confuses me is that this include should be exported by the controlplane dependency. I will try to take a look this week.

@grg
Copy link
Contributor Author

grg commented Feb 13, 2024

What confuses me is that this include should be exported by the controlplane dependency. I will try to take a look this week.

Any opinion on whether it's better with or without the 3rd commit?

@fruffy
Copy link
Collaborator

fruffy commented Feb 13, 2024

What confuses me is that this include should be exported by the controlplane dependency. I will try to take a look this week.

Any opinion on whether it's better with or without the 3rd commit?

I think it is better without. So far I am unable to reproduce this. I do not have a Mac.

@grg
Copy link
Contributor Author

grg commented Feb 14, 2024

What confuses me is that this include should be exported by the controlplane dependency. I will try to take a look this week.

Any opinion on whether it's better with or without the 3rd commit?

I think it is better without. So far I am unable to reproduce this. I do not have a Mac.

Here's what I had to change on a Mac to get it to build without the last commit:

diff --git a/backends/bmv2/CMakeLists.txt b/backends/bmv2/CMakeLists.txt
index 034e2a095..30c3cc492 100644
--- a/backends/bmv2/CMakeLists.txt
+++ b/backends/bmv2/CMakeLists.txt
@@ -84,6 +84,9 @@ set (IR_DEF_FILES ${IR_DEF_FILES} ${CMAKE_CURRENT_SOURCE_DIR}/bmv2.def PARENT_SC
 
 add_library(bmv2backend STATIC ${BMV2_BACKEND_COMMON_SRCS})
 add_dependencies(bmv2backend ir-generated frontend)
+target_include_directories(bmv2backend
+  SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS}
+)
 
 add_executable(p4c-bm2-ss ${BMV2_SIMPLE_SWITCH_SRCS})
 target_link_libraries (p4c-bm2-ss bmv2backend ${P4C_LIBRARIES} ${P4C_LIB_DEPS})
diff --git a/frontends/CMakeLists.txt b/frontends/CMakeLists.txt
index c2704d246..1c505c76b 100644
--- a/frontends/CMakeLists.txt
+++ b/frontends/CMakeLists.txt
@@ -240,6 +240,9 @@ add_parser(p4)
 
 add_library (frontend-parser-gen OBJECT ${p4PARSER_GEN_SRCS} ${v1PARSER_GEN_SRCS})
 add_dependencies (frontend-parser-gen mkp4dirs mkv1dirs ir-generated)
+target_include_directories(frontend-parser-gen
+  SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS}
+)
 
 set (FRONTEND_SOURCES
   ${COMMON_FRONTEND_SRCS}

@grg grg force-pushed the grgibb/control_plane_json_options branch 2 times, most recently from bc20148 to 376b687 Compare February 16, 2024 15:28
Expose JsonPrintOptions to allow JSON output format to be modified.
Move P4RuntimeFormat definition to a new header file to allow options.h
to include only that header file, and not everything in
p4RuntimeSerializer.h. This prevents needing to pull in the protobuf
includes for any file that uses options.h
@grg grg force-pushed the grgibb/control_plane_json_options branch from 376b687 to 441b614 Compare February 16, 2024 15:55
@grg grg merged commit bf9e7be into main Feb 16, 2024
16 checks passed
@grg grg deleted the grgibb/control_plane_json_options branch February 16, 2024 17:58
grg added a commit that referenced this pull request Apr 3, 2024
Expose JsonPrintOptions to allow JSON output format to be modified.

Move P4RuntimeFormat definition to a new header file to allow options.h
to include only that header file, and not everything in
p4RuntimeSerializer.h. This prevents needing to pull in the protobuf
includes for any file that uses options.h

* MEV backport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants