-
Notifications
You must be signed in to change notification settings - Fork 93
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 p4c from custom fork #249
base: main
Are you sure you want to change the base?
Conversation
|
||
RUN apt-get update && apt-get install -y --no-install-recommends $PI_DEPS $PI_RUNTIME_DEPS | ||
|
||
RUN cd / && git clone --depth=1 -b v1.43.2 https://github.com/google/grpc.git && \ |
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.
Instead of building gRPC, try using the technique here to use pre-built grpc libraries, it's much faster:
https://github.com/Azure/DASH/blob/main/dash-pipeline/dockerfiles/Dockerfile.bmv2-bldr#L3
https://github.com/Azure/DASH/blob/main/dash-pipeline/dockerfiles/Dockerfile.bmv2-bldr#L54
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.
Done
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.
Please see comments.
@@ -412,6 +415,19 @@ run-saithrift-client-bash: | |||
# --build-arg available_processors=$(shell nproc) \ | |||
# dockerfiles | |||
|
|||
# Builder, has base packages to make client docker | |||
docker-dash-p4c-bldr: |
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.
You also need to make target docker-pull-dash-p4c-bldr which will be called in CI scripts. (We docker pull
deliberately to make a failure more obvious.)
More importantly, you need to manually build and push this image to dockerhub for CI to pass, because the Dockerfile which uses this as a base cannot be found outside your PC environment, hence this CI failure:
https://github.com/Azure/DASH/actions/runs/3208306232/jobs/5244034990#step:3:570
I just made a new repo chrissommers/dash-p4c-bldr and added you as a collaborator. You can use this until we migrate to ACR.
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.
Please see comments concerning Dockerfiles, thanks.
Introduce p4c-bldr docker Use custom fork for building p4c that contains support for add-on-miss primitives from pna proposal Signed-off-by: Marian Pritsak <[email protected]>
@@ -0,0 +1,113 @@ | |||
# It uses grpc 1.43.2 as base to ensure right lib versions. | |||
FROM chrissommers/dash-grpc:1.43.2 as grpc |
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.
Please use sonicdash.azurecr.io/dash-grpc:1.43.2
@@ -43,6 +43,9 @@ DOCKER_SAITHRIFT_BLDR_IMG ?=chrissommers/dash-saithrift-bldr:220819 | |||
# Base image with test frameworks, DASH client libs not installed | |||
DOCKER_SAITHRIFT_CLIENT_BLDR_IMG ?=chrissommers/dash-saithrift-client-bldr:220819 | |||
|
|||
# P4c builder - p4c build dependencies, custom p4c fork | |||
DOCKER_P4C_BLDR_IMG ?=chrissommers/dash-p4c-bldr:221007 |
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.
Note, following #225 docker images are defined in .env
files under dockerfiles/
and are included
into the Makefile
. You should sync to main, thanks.
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.
See comments about Dockerfiles.
Needs to be updated to latest docker build (system) to use SHA for code tags provided by @lguohan - PR may need a refreshed look |
Hi @chrispsommers and @marian-pritsak - do we still need this PR? |
As far as I recall, @marian-pritsak had indicated in a previous WG meeting that work on this has been suspended, in anticipation of the P4-DPDK being a more viable long-term solution, primarily because it's already PNA-compliant, and thus better suited for stateful processing without fundamental changes. I would tend to agree with this analysis. |
Thanks Chris, I'll mark this as Suspended/Hold
From: Chris Sommers ***@***.***>
Sent: Wednesday, November 8, 2023 9:45 AM
To: sonic-net/DASH ***@***.***>
Cc: Kristina Moore ***@***.***>; Comment ***@***.***>
Subject: Re: [sonic-net/DASH] Build p4c from custom fork (PR #249)
As far as I recall, @marian-pritsak<https://github.com/marian-pritsak> had indicated in a previous WG meeting that work on this has been suspended, in anticipation of the P4-DPDK being a more viable long-term solution, primarily because it's already PNA-compliant, and thus better suited for stateful processing without fundamental changes. I would tend to agree with this analysis.
-
Reply to this email directly, view it on GitHub<#249 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFJSI6CLM4EYK7QJKZP2FHDYDOZHFAVCNFSM6AAAAAAQ77ZQFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGI3TANRQHE>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
Introduce p4c-bldr docker
Use custom fork for building p4c that contains support for add-on-miss primitives from pna proposal
Signed-off-by: Marian Pritsak [email protected]