Skip to content

Update name of Ubuntu 24.04 Docker image used as input - #669

Closed
jafingerhut wants to merge 22 commits into
p4lang:mainfrom
jafingerhut:update-docker-image-name-for-ubuntu-2404
Closed

Update name of Ubuntu 24.04 Docker image used as input#669
jafingerhut wants to merge 22 commits into
p4lang:mainfrom
jafingerhut:update-docker-image-name-for-ubuntu-2404

Conversation

@jafingerhut

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut
jafingerhut requested a review from fruffy September 2, 2026 06:17
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@github-actions github-actions Bot added the github_actions Pull requests that update GitHub Actions code label Sep 2, 2026
…ckages

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
ones that are more easily installable on Ubuntu 24.04

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Comment thread .github/workflows/test.yml Outdated
docker run -w /PI pi bash -c \
'apt-get update && apt-get install -y python3-pip && \
python3 -m pip install ptf scapy grpcio googleapis-common-protos protobuf==3.20 && \
'sudo apt-get update && sudo apt-get install -y build-essential python3-dev &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sudo is not available in docker iirc.

Looks like this should be part of the third-party image, if it is missing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the record, also okay to add it here, but likely should be in the build file.

Not when we run tests. If the tests fail, our image is incomplete

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice. Right now I am looking into some failures to compile the PI code when configure is used to enable all of the optional pieces. I have compiled PI with a subset of its options enabled for years on Ubuntu 24.04 and gotten it to pass tests when linked with simple_switch_grpc and run through all of p4c's tests, for example, but apparently there is PI C++ code that does not compile at all with the default version of GCC installed by apt on Ubuntu 24.04, that I haven't ever tried to compile before now.

We are experiencing this because either (a) no one has ever tried this before and seen the failures, or (b) they have experienced them, but haven't bothered to report them.

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut

Copy link
Copy Markdown
Contributor Author

@fruffy This should be passing all tests now.

I also did these changes:

  • updated the version of clang tested. The older ones did not even have apt packages available on Ubuntu 24.04
  • disabled --with-proto configure option in Dockerfile.bmv2. Enabling that causes compile-time errors with some source files that I don't know to fix at this time.
  • disabled running of ./tools/check_style.sh at the end. There are failures there that I haven't looked into how to fix, but the better answer there is probably to use code style/lint checking tools that are used in other C++ and Python code in other p4lang projects.

Comment thread .github/workflows/test.yml Outdated
strategy:
matrix:
include:
- cxx: clang++-18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make this generic like we do with gcc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 16, passes CI tests still.

docker run -w /PI pi bash -c \
'apt-get update && apt-get install -y python3-pip && \
python3 -m pip install ptf scapy grpcio googleapis-common-protos protobuf==3.20 && \
'apt-get update && apt-get install -y build-essential python3-dev &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be part of the build step, not the test step. If the tests require this to pass we are shipping a broken image

Comment thread Dockerfile.bmv2
apt-get install -y --no-install-recommends $PI_DEPS $PI_RUNTIME_DEPS && \
./autogen.sh && \
./configure --enable-Werror --with-bmv2 --with-proto --with-cli --with-internal-rpc --with-sysrepo && \
# TODO: Change --without-proto to --with-proto when we learn how

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the actual error here? Maybe it is a trivial fix

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can see the errors in the logs of this similar PR, which I created in parallel to try out some variations on this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have since found fairly small changes to the C++ code to keep the proto option enabled during PI conigure step. The changes are in the PR #670, which I suspect may replace this one soon. I plan to make a few more commits to #670 soon to try to move the installation of Python packages into the build step rather than the test step, as you commented on for this PR.

instead of picking a particular version number

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
…lures

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut

Copy link
Copy Markdown
Contributor Author

Closing this PR, as I recommend reviewing PR #670 instead.

@jafingerhut jafingerhut closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants