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 p4c from custom fork #249

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marian-pritsak
Copy link
Collaborator

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]


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 && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@chrispsommers chrispsommers left a 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:
Copy link
Collaborator

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.

Copy link
Collaborator

@chrispsommers chrispsommers left a 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
Copy link
Collaborator

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
Copy link
Collaborator

@chrispsommers chrispsommers Nov 3, 2022

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.

Copy link
Collaborator

@chrispsommers chrispsommers left a 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.

@KrisNey-MSFT
Copy link
Collaborator

KrisNey-MSFT commented Jan 19, 2023

Needs to be updated to latest docker build (system) to use SHA for code tags provided by @lguohan - PR may need a refreshed look

@KrisNey-MSFT
Copy link
Collaborator

Hi @chrispsommers and @marian-pritsak - do we still need this PR?
TY, Kristina

@chrispsommers
Copy link
Collaborator

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.

@KrisNey-MSFT
Copy link
Collaborator

KrisNey-MSFT commented Nov 8, 2023 via email

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.

3 participants