-
Notifications
You must be signed in to change notification settings - Fork 68
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
Update networking container to fix unused p4c flag & add new dependencies #395
Conversation
Signed-off-by: bharticemk <richa.bharti@intel.com>
206e41f
to
6c16ed0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -94,7 +94,6 @@ git submodule update --init --recursive | |||
mkdir build && mkdir -p "${WORKING_DIR}"/p4c/install && cd build | |||
|
|||
cmake -DCMAKE_INSTALL_PREFIX="${WORKING_DIR}"/p4c/install \ | |||
-DENABLE_BMV2=OFF \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the reason for build failures in container build. Confirmed by the p4c team and needs to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest P4 Compiler uses some code from bmv2 , so it shouldn't be disabled
Hence they asked to remove this in build script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the reason for build failures in container build. Confirmed by the p4c team and needs to be removed
What were the build failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error Logs:
[ 87%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir/midend.cpp.o
In file included from /root/P4C/backends/bmv2/common/helpers.h:22,
from /root/P4C/backends/bmv2/common/action.h:20,
from /root/P4C/backends/bmv2/psa_switch/psaSwitch.h:20,
from /root/P4C/backends/ebpf/psa/ebpfPsaGen.h:20,
from /root/P4C/backends/tc/backend.h:20,
from /root/P4C/backends/tc/ebpfCodeGen.h:20,
from /root/P4C/backends/tc/ebpfCodeGen.cpp:17:
/root/P4C/backends/bmv2/common/expression.h:125:30: error: 'IntMod' in namespace 'IR' does not name a type
125 | void postorder(const IR::IntMod expression) override;
| ^~~~~~
/root/P4C/backends/bmv2/common/expression.h:125:10: error: 'void BMV2::ExpressionConverter::postorder(const int)' marked 'override', but does not override
125 | void postorder(const IR::IntMod *expression) override;
| ^~~~~~~~~
In file included from /root/P4C/backends/bmv2/common/helpers.h:22,
from /root/P4C/backends/bmv2/common/action.h:20,
from /root/P4C/backends/bmv2/psa_switch/psaSwitch.h:20,
from /root/P4C/backends/ebpf/psa/ebpfPsaGen.h:20,
from /root/P4C/backends/tc/backend.h:20,
from /root/P4C/backends/tc/backend.cpp:17:
/root/P4C/backends/bmv2/common/expression.h:125:30: error: 'IntMod' in namespace 'IR' does not name a type
125 | void postorder(const IR::IntMod expression) override;
| ^~~~~~
/root/P4C/backends/bmv2/common/expression.h:125:10: error: 'void BMV2::ExpressionConverter::postorder(const int)' marked 'override', but does not override
125 | void postorder(const IR::IntMod *expression) override;
| ^~~~~~~~~
[ 87%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir/pnaProgramStructure.cpp.o
[ 87%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir/tc.cpp.o
[ 88%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir/tcAnnotations.cpp.o
[ 88%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir/version.cpp.o
[ 89%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir//ebpf/ebpfBackend.cpp.o
[ 89%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir//ebpf/ebpfProgram.cpp.o
[ 89%] Building CXX object backends/tc/CMakeFiles/p4c-pna-p4tc.dir//ebpf/ebpfTable.cpp.o
make[2]: *** [backends/tc/CMakeFiles/p4c-pna-p4tc.dir/build.make:95: backends/tc/CMakeFiles/p4c-pna-p4tc.dir/ebpfCodeGen.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [backends/tc/CMakeFiles/p4c-pna-p4tc.dir/build.make:82: backends/tc/CMakeFiles/p4c-pna-p4tc.dir/backend.cpp.o] Error 1
In file included from /root/P4C/backends/bmv2/common/backend.h:22,
from /root/P4C/backends/tc/pnaProgramStructure.h:20,
from /root/P4C/backends/tc/pnaProgramStructure.cpp:17:
/root/P4C/backends/bmv2/common/expression.h:125:30: error: 'IntMod' in namespace 'IR' does not name a type
125 | void postorder(const IR::IntMod expression) override;
| ^~~~~~
/root/P4C/backends/bmv2/common/expression.h:125:10: error: 'void BMV2::ExpressionConverter::postorder(const int)' marked 'override', but does not override
125 | void postorder(const IR::IntMod *expression) override;
| ^~~~~~~~~
In file included from /root/P4C/backends/bmv2/common/helpers.h:22,
from /root/P4C/backends/bmv2/common/action.h:20,
from /root/P4C/backends/bmv2/psa_switch/psaSwitch.h:20,
from /root/P4C/backends/ebpf/psa/ebpfPsaGen.h:20,
from /root/P4C/backends/tc/backend.h:20,
from /root/P4C/backends/tc/tc.cpp:17:
/root/P4C/backends/bmv2/common/expression.h:125:30: error: 'IntMod' in namespace 'IR' does not name a type
125 | void postorder(const IR::IntMod expression) override;
| ^~~~~~
/root/P4C/backends/bmv2/common/expression.h:125:10: error: 'void BMV2::ExpressionConverter::postorder(const int)' marked 'override', but does not override
125 | void postorder(const IR::IntMod *expression) override;
| ^~~~~~~~~
In file included from /root/P4C/backends/bmv2/common/helpers.h:22,
from /root/P4C/backends/bmv2/common/action.h:20,
from /root/P4C/backends/bmv2/psa_switch/psaSwitch.h:20,
from /root/P4C/backends/ebpf/ebpfBackend.cpp:19:
/root/P4C/backends/bmv2/common/expression.h:125:30: error: 'IntMod' in namespace 'IR' does not name a type
125 | void postorder(const IR::IntMod expression) override;
| ^~~~~~
/root/P4C/backends/bmv2/common/expression.h:125:10: error: 'void BMV2::ExpressionConverter::postorder(const int)' marked 'override', but does not override
125 | void postorder(const IR::IntMod *expression) override;
| ^~~~~~~~~
make[2]: *** [backends/tc/CMakeFiles/p4c-pna-p4tc.dir/build.make:134: backends/tc/CMakeFiles/p4c-pna-p4tc.dir/pnaProgramStructure.cpp.o] Error 1
[ 89%] Linking CXX executable p4c-dpdk
make[2]: *** [backends/tc/CMakeFiles/p4c-pna-p4tc.dir/build.make:186: backends/tc/CMakeFiles/p4c-pna-p4tc.dir//ebpf/ebpfBackend.cpp.o] Error 1
[ 89%] Built target p4c-dpdk
make[2]: *** [backends/tc/CMakeFiles/p4c-pna-p4tc.dir/build.make:147: backends/tc/CMakeFiles/p4c-pna-p4tc.dir/tc.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1537: backends/tc/CMakeFiles/p4c-pna-p4tc.dir/all] Error 2
make: *** [Makefile:182: all] Error 2
The command '/bin/sh -c /root/scripts/distro_pkg_install.sh --install-dev-pkgs --scripts-dir=/root/scripts && /root/scripts/install_nr_modules.sh --workdir=/root && /root/scripts/post_build_cleanup.sh --workdir=/root --keep-source-code="${KEEP_SOURCE_CODE}" --deployment-image="${DEPLOYMENT_IMAGE}"' returned a non-zero code: 1
has context menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address two build issues:
- Missing 'netaddr' module. Updated install script to provide it.
- Compile errors in P4C. The latest P4C compiler uses some code from bmv2 , so it shouldn't be disabled. Removed ENABLE_BMV2=OFF from the cmake command. Verified with compiler team that this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This provides a concise explanation of each problem and an explanation of the solution. In the case of the P4C compile errors, you've shared the information that solution was confirmed by the code's authors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The critical one here is the change to the ENABLE_BMV2 parameter. The reason for adding the 'netaddr' module can reasonably be induced. The ENABLE_BMV2 is considerably more subtle. The most important reason for adding a comment is to answer the question, "Why?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update the CL comment to say why these changes are being made (what problems this solves)?
The changelist description (the text box at the top of this page) should provide enough information for the reviewers to understand the change and judge whether the CL achieves its purpose. See Writing Good CL Descriptions for an explanation and some tips. |
* Update ipdk_v23.07 release notes on main branch Signed-off-by: Sabeel Ansari <sabeel.ansari@intel.com> * Update p4c, psde and infrap4d SHA for 23.07 release Signed-off-by: Sabeel Ansari <sabeel.ansari@intel.com> * update distro_pkg_install.sh and build_p4c.sh (#395) Address two build issues: - Missing 'netaddr' module. Updated install script to provide it. - Compile errors in P4C. The latest P4C compiler uses some code from bmv2 , so it shouldn't be disabled. Removed ENABLE_BMV2=OFF from the cmake command. Verified with compiler team that this is correct. Signed-off-by: bharticemk <richa.bharti@intel.com> --------- Signed-off-by: Sabeel Ansari <sabeel.ansari@intel.com> Signed-off-by: bharticemk <richa.bharti@intel.com> Co-authored-by: Filip Szufnarowski <filip.szufnarowski@intel.com> Co-authored-by: Richa Bharti <102134627+bharticemk@users.noreply.github.com>
Address two build issues: