Run build and tests directly on Ubuntu 20.04, 22.04 without docker #3770

davidbolvansky · 2022-12-10T18:29:06Z

No description provided.

fruffy · 2022-12-10T20:00:56Z

https://github.com/p4lang/third-party/blob/main/Dockerfile contains most of the dependencies. You will also have to install protobuf, thrift, grpc, nanomsg etc. I am not sure if the bmv2 package provides these. But with newer versions they can easily be installed using debian.

davidbolvansky · 2022-12-10T20:06:01Z

https://github.com/p4lang/third-party/blob/main/Dockerfile contains most of the dependencies. You will also have to install protobuf, thrift, grpc, nanomsg etc. I am not sure if the bmv2 package provides these. But with newer versions they can easily be installed using debian.

probably they are installed as building works fine on Ubuntu 22.

davidbolvansky · 2022-12-10T20:55:36Z

https://github.com/p4lang/third-party/blob/main/Dockerfile contains most of the dependencies. You will also have to install protobuf, thrift, grpc, nanomsg etc. I am not sure if the bmv2 package provides these. But with newer versions they can easily be installed using debian.

probably they are installed as building works fine on Ubuntu 22.

but ptf is missing. So lets switch to ubuntu only for normal tests, default to existing base image.

fruffy · 2022-12-10T21:15:59Z

but ptf is missing. So lets switch to ubuntu only for normal tests, default to existing base image.

Isn't this install just pip3 install --user ptf?

fruffy · 2022-12-10T21:47:51Z

I think it is also fine to just test the build for Ubuntu 18.04 and not run tests since there are limited guarantees.

davidbolvansky · 2022-12-11T00:34:01Z

I think it is also fine to just test the build for Ubuntu 18.04 and not run tests since there are limited guarantees.

I was able to run them so it would be great to keep them for some time. If p4c project after April 2023 decides to drop this CI support for U18 with reasonable reasons, I am fine with that.

davidbolvansky · 2022-12-11T00:35:15Z

ir/ir-inline.h

@@ -269,7 +269,7 @@ void IR::NameMap<T, MAP, COMP, ALLOC>::visit_children(Visitor& v) {
                namemap_insert_helper(i, cstring(obj_name(s)), std::move(s), symbols, new_symbols);
                i = symbols.erase(i);
            }
-        } else {
+        } else if (n) {


new gcc (false positive) warnings..

What is the warning?

that n maybe be null pointer (not really, sanitizers would catch it)

Instead of the if checks, we should use a BUG_CHECK here.

inline.patch.txt

How about this patch?

But does not happen in practice as sanitizers would catch for sure.

Well, only if we have written the appropriate tests/P4-programs. :)

Perhaps we should open a separate pull request with these changes?

I dont think we need another PR as this PR enables more testing, especially on Ubuntu 22, where we have newer GCC which emits more warnings. Seems OK to me to have such fixes in this PR.

Tests now fail - expected?

Sorry but I would really prefer first version of these changes (my) as your changes seems to break existing tests and I am not going to debug them.

rst0git · 2022-12-11T08:33:12Z

You will also have to install protobuf, thrift, grpc, nanomsg etc. I am not sure if the bmv2 package provides these.

These are listed as dependencies for the bmv2 package.

fruffy · 2022-12-11T17:39:11Z

.github/workflows/ci-test.yml

+
+    - name: Build (Ubuntu 22.04, GCC)
+      run: |
+        docker build -t p4c --build-arg UBUNTU_VERSION=22.04 --build-arg IMAGE_TYPE=test --build-arg ENABLE_UNIFIED_COMPILATION=ON .


Do you even need the docker image at this point?

Well, good point, probably not.

fruffy · 2022-12-11T17:39:54Z

tools/ci-build.sh


 # we want to use Python as the default so change the symlinks
 ln -sf /usr/bin/python3 /usr/bin/python
 ln -sf /usr/bin/pip3 /usr/bin/pip

+pip3 install --upgrade pip
 pip3 install wheel


wheel might not be needed anymore.

fruffy · 2022-12-11T17:40:23Z

ir/ir-inline.h

@@ -269,7 +269,7 @@ void IR::NameMap<T, MAP, COMP, ALLOC>::visit_children(Visitor& v) {
                namemap_insert_helper(i, cstring(obj_name(s)), std::move(s), symbols, new_symbols);
                i = symbols.erase(i);
            }
-        } else {
+        } else if (n) {


What is the warning?

rst0git · 2022-12-11T17:55:07Z

Dockerfile

-FROM p4lang/behavioral-model:latest
+ARG UBUNTU_VERSION=20.04
+ARG BASE_IMAGE=ubuntu:${UBUNTU_VERSION}
+FROM ${BASE_IMAGE}


I think we need to keep p4lang/behavioral-model:latest as default base image.
This Dockerfile is used to automatically build and publish the p4c container image in Docker Hub and this container image is being used in other projects (e.g., p4app).

We could keep this docker file for the release build. With David's new setup we might not need a docker file at all for the p4c image.

fruffy · 2022-12-14T14:38:26Z

.github/workflows/ci-test.yml

      run: |
-        docker build -t p4c --build-arg IMAGE_TYPE=test --build-arg ENABLE_UNIFIED_COMPILATION=ON --build-arg COMPILE_WITH_CLANG=ON \
-        --build-arg BUILD_AUTO_VAR_INIT_PATTERN=ON --build-arg ENABLE_SANITIZERS=ON .
+        docker build -t p4c --build-arg BASE_IMAGE=ubuntu:18.04 --build-arg IMAGE_TYPE=test --build-arg ENABLE_UNIFIED_COMPILATION=ON .


This also needs to be updated to use Ubuntu 18.04 directly. Also other images such as the tools test should not use docker anymore then.

There is no Ubuntu 18.04 image for github actions. So nope, not possible.

But we do not modify the base image right now so this will test on the Ubuntu 20.04 docker image as far as I can see.

Also other images such as the tools test should not use docker anymore then.

Sorry but for now I am not gonna do everything. We can move it incrementally.

The issue is that the new ci-build configuration seems to break the ccache that has been set up for the docker build process. As far as I can tell, the builds are taking longer now. We can check this by completing one build and rerunning to see whether the build is cached.

Build on Ubuntu 22 - 6m 14s.

Seems pretty OK to me.

The problem is the other tests, static-build-test-p4c, test-tools, ptf-ebpf, validate etc.

I can convert them but then we would stop testing with Dockerfile so it can slowly bitrot.

Yes, but this PR is breaking the ccache setup for all other tests.

How? Could you explain it?

Docker builds uses export_ccache.sh which runs
docker cp $(docker create --rm p4c):/p4c/.ccache .
, non-docker runs
sudo cp -rf /p4c/.ccache .

I pushed empty commit to see if regressions are real.

Could be this change.

The problem is the other tests, static-build-test-p4c, test-tools, ptf-ebpf, validate etc.

I can convert them but then we would stop testing with Dockerfile so it can slowly bitrot.

We can either convert them or have a workaround. As Radostin mentioned we need to create one docker build either way.

In general, test-tools will now be the slowest test because the build alone takes 55 minutes.

ccache is working fine, rebuild time: 7m 25s

https://github.com/p4lang/p4c/actions/runs/3697344554/jobs/6262220551

fruffy · 2022-12-14T14:41:20Z

.github/workflows/ci-test.yml

-        ./tools/export_ccache.sh
+        sudo cp -rf . /p4c/
+        (cd /p4c/ && sudo -E tools/ci-build.sh)
+        sudo cp -rf /p4c/.ccache .


Both the cp commands should not be necessary to get ccache to work.

ci-build.sh uses /p4c/

We would be more flexible after we drop docker, but for now, things needs to be done this way, probably.

I will think about it but probably I would leave it for followup, as this change is quite huge anyway.

davidbolvansky · 2022-12-14T17:55:25Z

Based on new runs, I believe ccache is working fine, rebuild times are short.

davidbolvansky · 2022-12-14T21:34:29Z

CI also significatly faster.

for example sanitizers 120min -> 71 min.

My assumption is that CTEST_PARALLEL_LEVEL was not handled correctly (when you run something with sudo you lose env vars, you need to use -E switch)

davidbolvansky · 2022-12-15T01:01:45Z

Ok, done

fruffy · 2022-12-15T07:54:19Z

Looks good. The question is what we do with the docker build process that publishes builds to dockerhub. Have a nightly test run?

davidbolvansky · 2022-12-15T10:54:32Z

Or we can preserve testing with docker on U20. I dont know..

fruffy · 2022-12-15T11:01:01Z

@rst0git any thoughts on this?

rst0git · 2022-12-15T16:34:28Z

The question is what we do with the docker build process that publishes builds to dockerhub.

With the following change, when BASE_IMAGE is not specified, the container image should be the same.

-FROM p4lang/behavioral-model:latest
+ARG BASE_IMAGE=p4lang/behavioral-model:latest
+FROM ${BASE_IMAGE}

I think with these changes the CI workflow updating the container image in Dockerhub should continue to work as expected.

fruffy · 2022-12-16T10:29:27Z

The question is what we do with the docker build process that publishes builds to dockerhub.

With the following change, when BASE_IMAGE is not specified, the container image should be the same.
-FROM p4lang/behavioral-model:latest
+ARG BASE_IMAGE=p4lang/behavioral-model:latest
+FROM ${BASE_IMAGE}
I think with these changes the CI workflow updating the container image in Dockerhub should continue to work as expected.

It'd be nice if we ran tests on this release build. This way we can make sure the docker build does not rot.

fruffy · 2022-12-16T10:33:58Z

@antoninbas @mbudiu-vmw

This PR would remove the docker third-party build from most of the tests. Any further concerns?

antoninbas · 2022-12-16T18:39:21Z

no concern from me, but then I am not the one maintaining this

rst0git · 2022-12-16T18:47:29Z

It'd be nice if we ran tests on this release build. This way we can make sure the docker build does not rot.

If something with the container image ever breaks, we can introduce the necessary tests.

mihaibudiu · 2022-12-16T22:34:07Z

I am not maintaining this either

davidbolvansky · 2022-12-16T22:38:10Z

It'd be nice if we ran tests on this release build. This way we can make sure the docker build does not rot.

If something with the container image ever breaks, we can introduce the necessary tests.

Yes, exactly.

davidbolvansky · 2022-12-16T22:54:06Z

@fruffy is it ok for you?

rst0git · 2022-12-17T11:11:57Z

I am not maintaining this either

I would be happy to be a maintainer.

davidbolvansky changed the title ~~Rework base of Dockerfile (WIP)~~ Rework base of Dockerfile, run build and tests on 22.04 as well. Dec 10, 2022

davidbolvansky changed the title ~~Rework base of Dockerfile, run build and tests on 22.04 as well.~~ Rework base of Dockerfile, run build and tests on 22.04 as well Dec 10, 2022

davidbolvansky mentioned this pull request Dec 10, 2022

CI: also run on Ubuntu 20 and 18 #3749

Closed

davidbolvansky changed the title ~~Rework base of Dockerfile, run build and tests on 22.04 as well~~ Change base image of Dockerfile to Ubuntu, run build and tests on Ubuntu 22.04 as well Dec 10, 2022

davidbolvansky changed the title ~~Change base image of Dockerfile to Ubuntu, run build and tests on Ubuntu 22.04 as well~~ Run build and tests on Ubuntu 22.04 as well Dec 10, 2022

davidbolvansky changed the title ~~Run build and tests on Ubuntu 22.04 as well~~ Run build and tests on Ubuntu 22.04 Dec 10, 2022

davidbolvansky marked this pull request as ready for review December 11, 2022 00:34

davidbolvansky requested a review from fruffy December 11, 2022 00:34

davidbolvansky changed the title ~~Run build and tests on Ubuntu 22.04~~ Run build and tests on Ubuntu 18.04, 22.04 Dec 11, 2022

davidbolvansky commented Dec 11, 2022

View reviewed changes

Rework base of Dockerfile

b6fc886

davidbolvansky requested a review from rst0git December 11, 2022 14:29

fruffy reviewed Dec 11, 2022

View reviewed changes

rst0git reviewed Dec 11, 2022

View reviewed changes

run tests directly

0505bc2

davidbolvansky requested a review from fruffy December 14, 2022 11:44

fruffy reviewed Dec 14, 2022

View reviewed changes

speculative fixes

872d0fe

davidbolvansky changed the title ~~Run build and tests on Ubuntu 18.04, 22.04~~ Run build and tests directly on Ubuntu 18.04, 22.04 without docker Dec 14, 2022

davidbolvansky changed the title ~~Run build and tests directly on Ubuntu 18.04, 22.04 without docker~~ Run build and tests directly on Ubuntu 20.04, 22.04 without docker Dec 14, 2022

Empty commit for ccache test

89f8365

mm

660a080

davidbolvansky added 3 commits December 14, 2022 19:57

ptf

7a9aae0

lint

63b9e68

up

e466e3e

davidbolvansky requested a review from fruffy December 14, 2022 19:47

davidbolvansky added 2 commits December 14, 2022 22:35

static build

830909f

simplify

c2caff5

check null

50aa2cc

ir-inline patchup.

f16c832

fruffy requested a review from mihaibudiu December 16, 2022 10:32

rst0git approved these changes Dec 16, 2022

View reviewed changes

rst0git mentioned this pull request Dec 17, 2022

Deprecate unified build in favor of unity build. #3491

Merged

davidbolvansky merged commit 9bfb5a1 into p4lang:main Dec 17, 2022

davidbolvansky deleted the dbolvans/docker_ubuntu branch December 17, 2022 13:21

Run build and tests directly on Ubuntu 20.04, 22.04 without docker #3770

Run build and tests directly on Ubuntu 20.04, 22.04 without docker #3770

Conversation

davidbolvansky commented Dec 10, 2022

fruffy commented Dec 10, 2022

davidbolvansky commented Dec 10, 2022

davidbolvansky commented Dec 10, 2022 • edited Loading

fruffy commented Dec 10, 2022

fruffy commented Dec 10, 2022 • edited Loading

davidbolvansky commented Dec 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rst0git commented Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rst0git Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbolvansky Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

davidbolvansky commented Dec 14, 2022

davidbolvansky commented Dec 14, 2022 • edited Loading

davidbolvansky commented Dec 15, 2022

fruffy commented Dec 15, 2022

davidbolvansky commented Dec 15, 2022

fruffy commented Dec 15, 2022

rst0git commented Dec 15, 2022

fruffy commented Dec 16, 2022

fruffy commented Dec 16, 2022

antoninbas commented Dec 16, 2022

rst0git commented Dec 16, 2022

mihaibudiu commented Dec 16, 2022

davidbolvansky commented Dec 16, 2022

davidbolvansky commented Dec 16, 2022

rst0git commented Dec 17, 2022

davidbolvansky commented Dec 10, 2022 •

edited

Loading

fruffy commented Dec 10, 2022 •

edited

Loading

rst0git commented Dec 11, 2022 •

edited

Loading

rst0git Dec 11, 2022 •

edited

Loading

davidbolvansky Dec 14, 2022 •

edited

Loading

davidbolvansky commented Dec 14, 2022 •

edited

Loading