Force CPP++17 installation for Thrift and Boost.

[fruffy]

Fixes #8.

[jafingerhut]

@vgurevich These changes update Thrift from version 0.14 to 0.21, which I think were released about 3.5 years apart. I do not know the Thrift developers policies on maintaining compatibility across versions, but it seems worth running at least one test that exercises Thrift to test such a change. Do you knew whether:

(a) running a test like the tna_counters one exercises using the Thrift API at all?
(b) Is there another test in this repository that you know does exercise Thrift?

[vgurevich]

@jafingerhut -- P4_16 tests do not use Thrift. They use gRPC. All P4_14 tests use Thrift. Also it is used by bf-diags and SAI

[jafingerhut]

I will try building with this PR's changes locally, and then try out a P4_14 test or two to check whether they pass, and if so, how long they take, and suggest one for adding to the CI build & test.

[jafingerhut]

@fruffy I successfully built on an Ubuntu 20.04 freshly installed VM with this PR's changes. I am still testing a few more P4_14 tests, but have results for most of them here: https://docs.google.com/spreadsheets/d/1WCP2J7_g4j3ilFVRXaEM9S39_v6Go1_5WcCYJ6uJYY4/edit?gid=0#gid=0

I will add results for trying the P4_16 tests after that.

The few P4_14 tests that are failing are due to one of two reasons, so far:

(a) missing configuration files. I think they are tests where they are intended to have the user write a JSON config file to direct exactly how the test behaves, and no default version of those files were included in the repository (and maybe aren't in the original repository at Intel, either). I have checked, and they are not part of the private open-p4studio-old repository.

(b) One test named 'knet_mgr_test' has a Python ptf program that tries to use a class Packet that has not been imported. It is possible that this might be available in the scapy package, but not the bf_pktpy (partial) substitute package -- I have not dug into the details.

As far as I can tell, none of the failures are because of issues with Thrift, so that is good.

I recommend adding to this PR, in file .github/workflows/ci-test-ubuntu.yml, after these existing lines that run one P4_16 test: https://github.com/p4lang/open-p4studio/blob/main/.github/workflows/ci-test-ubuntu.yml#L67C11-L70C93

the following new lines, to run one P4_14 test that takes less than 40 sec on my system. Or we might want to put the existing test and the new one in a bash for loop, to make it easier to add new ones later, perhaps. Note that the killall commands, or something like them, are needed to kill the tofino-model and bf_switchd processes started by the first test, and still running at that point in time.

sudo killall bf_switchd
sudo killall tofino-model

TESTNAME="mirror_test"
sudo ${SDE_INSTALL}/bin/veth_setup.sh 128
          ./run_tofino_model.sh -p ${TESTNAME} --arch tofino -q |& sed 's/^/model: /' &
          ./run_switchd.sh -p ${TESTNAME} --arch tofino |& sed 's/^/switchd: /' &
          timeout 10800 ./run_p4_tests.sh -p ${TESTNAME} --arch tofino |& sed 's/^/tests: /'

[fruffy]

I wonder whether we should even bother maintaining P4-14? I have been thinking of deprecating that code entirely for P4C to reduce the complexity of the code base.

[jafingerhut]

I wonder whether we should even bother maintaining P4-14? I have been thinking of deprecating that code entirely for P4C to reduce the complexity of the code base.

I think @vgurevich probably has a better view of how useful it is to keep it working. Dropping test support at some point seems reasonable, but given how new this repository is, it seems worth putting forth some minimal effort to see if it is working now, before explicitly saying "from date X forward, we are no longer testing P4_14 support".

[vgurevich]

@fruffy -- let's make sure we can make everything run first. There are still plenty of projects and publications that use (used) P4_14, including the number of those who had done it on Tofino. Do we want to leave all these people "high and dry"?

Also, as I mentioned above, the same API framework (PD API) is also used by bf-diags. Some of the APIs available through Thrift are still not available via BRI. So, even if you kill P4_14, it is not going to help you to remove Thrift.

[fruffy]

@fruffy -- let's make sure we can make everything run first. There are still plenty of projects and publications that use (used) P4_14, including the number of those who had done it on Tofino. Do we want to leave all these people "high and dry"?

Also, as I mentioned above, the same API framework (PD API) is also used by bf-diags. Some of the APIs available through Thrift are still not available via BRI. So, even if you kill P4_14, it is not going to help you to remove Thrift.

Not proposing to get rid of Thrift but proposing to get rid of P4-14. At some point we have to progress and there is a lot of code dedicated to P4-14 which can make changes to the infrastructure hard. We are not Microsoft who can afford the backwards compatibility and there are no paying customers here. I am actually quite okay with leaving users who refuse to move on high and dry. I even assume these users are still using some old version of the SDE anyway.

But this is something for the future. Right now, it probably makes sense to get one working version done. The Thrift changes here are mostly for making sure that we can compile bf-diags for Ubuntu 24.04 and C++17 (which most of the code base uses now).

[jafingerhut]

If/when we drop P4_14 support, it would be good to create a tag with the latest version that had P4_14 support, and document that tag in an easy-to-find place, e.g. top level README or some document linked from there, with test results.

[fruffy]

If/when we drop P4_14 support, it would be good to create a tag with the latest version that had P4_14 support, and document that tag in an easy-to-find place, e.g. top level README or some document linked from there, with test results.

Yes, totally!

[jafingerhut]

Most P4_14 tests pass, and the only failures are for reasons other than the Thrift library. Most P4_16 tests pass as well, and the few failures appear to have nothing to do with the Thrift library.

[jafingerhut]

@fruffy I have approved based on earlier testing, before your latest changes. Do you want me to retest before you merge? I am fine either way.

[fruffy]

@fruffy I have approved based on earlier testing, before your latest changes. Do you want me to retest before you merge? I am fine either way.

Not necessary. I might do some config tweaks to simplify and speed up the Thrift installation. That's it.

[fruffy]

Btw, we should really automate these kinds of tests that you do not have to do this every time.

[jafingerhut]

Btw, we should really automate these kinds of tests that you do not have to do this every time.

I certainly won't be doing them every time. Once things settle down and we trust the CI tests to catch most things, I'll probably try installing on a freshly installed system once per month or so, maybe even create a VirtualBox VM image for people to download if they don't want to build the code themselves.

I have forgotten exactly which recent PRs we were seeing differences between CI test results and my local install, but I haven't seen those in a while now.