p4lang
Remove unified build
The custom unified build code might actually not be needed anymore. CMAKE has added UNITY_BUILDS, which seem equivalent.
I left the library in the cmake folder, in case downstream repositories still needed. There is only one build testing that the non-unity build compiles, which reduces the amount of CI runs. I can also make unity builds the default if requested.
This requires fixes on some downstream repositories but otherwise works well. We may even drop the CI option to simplify tests.
Does this work with any version of cmake? Is it faster or comparable to the old unified build?
It works with CMake 3.16+, wich is the default since Ubuntu 20.04. The build times are roughly the same, but can be tweaked with UNITY_BUILD_BATCH_SIZE.
I personally don't have a problem with this, but I think this should work on as many platforms as possible. If this precludes people running older versions of the software from building, then it could be problematic.
I guess it boils down how we want to maintain backwards compatibility. We dropped support for anything lower than Ubuntu 20.04 with this pull request. So it might make sense to use features that are provided by it.
The original unified_build library is still there. I can try to create a version that uses it depending on the CMake version. I was also hoping to simplify the CI with this PR. We do not need to test all combinations of unified and gmp builds.
"We dropped support for anything lower than Ubuntu 20.04 with https://github.com/p4lang/third-party/issues/25 pull request."
Techically, we stopped doing automated CI testing for anything lower than Ubuntu 20.04 with that change.
There was a commit in the last month or two that broke the build for Ubuntu 18.04, which when pointed out later via an issue then caused someone to create another PR that restored a working build for Ubuntu 18.04.
I agree that at some point it definitely makes sense not to support Ubuntu 18.04. A reasonable question is: when? One choice is right now. Another would be to wait until 5 years after Ubuntu 18.04 was released in April 2023, when Ubuntu discontinues free support for it.
The problem is that without CI we can not make any guarantees for 18.04. It will be quite hard to respect 18.04 restrictions going forward. This is related to the discussion in https://github.com/p4lang/p4c/issues/3451. Iirc that change worked because there was a solution that works for both 18.04 and future systems. This might not always be possible.
For this particular PR, I can try to come up with a solution that can also work with older CMake systems.
It works with CMake 3.16+, wich is the default since Ubuntu 20.04. The build times are roughly the same, but can be tweaked with
UNITY_BUILD_BATCH_SIZE.
What is the plan now? Land this PR after April 2023? Or just land it now and one (on U18) can install newer cmake easily for example with pip install cmake==3.16.3.
What is the plan now? Land this PR after April 2023? Or just land it now and one (on U18) can install newer cmake easily for example with pip install cmake==3.16.3.
I will have to revive this PR but I am not quite sure what to do with build_unified yet. I will have to implement an equivalent version so that downstream projects can still use it. It might be nicer if that explicit function were not necessary.
I have no objections to merging updates that require specific later versions of cmake on Ubuntu 18.04, as long as there are clear README directions on building on Ubuntu 18.04 somewhere.
build_unified
I would remove it.
Could you create new cmake option so we could try new cmake native unified build downstream and do necessary changes?
For now please pip install cmake 3.16.3 here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L98
Please fix failing CI - otherwise I see no blockers to proceed with this PR, do you?
@mbudiu-vmw @vhavel This PR needs authorized approval because it touches some Testgen code.
CI and cmake are not my speciality, so I'd rather have someone else comment.
CI and cmake are not my speciality, so I'd rather have someone else comment.
The issue is that this PR can not be merged unless someone of the code owners approves. It is a little unfortunate.
@vhavel @pkotikal This PR is stuck because of the reviewer lock we have on testgen files. If you have some time could you give this a review? This PR deprecates our homecooked unified build in favor of CMake's unity builds.
This looks fine, but I really don't know whether the new requirements will prevent people with older software from building the compiler.
We do have a CI run for Ubuntu 18, which already requires some workarounds. Ubuntu 16.04 we dropped support a long time ago. I do not know about other systems.
@mbudiu-vmw @vlstill and @davidbolvansky both approved but I can not merge this PR since it is missing an authoritative approval.
