Deprecate unified build in favor of unity build. #3491

fruffy · 2022-08-18T11:52:57Z

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.

fruffy · 2022-08-18T12:24:50Z

This requires fixes on some downstream repositories but otherwise works well. We may even drop the CI option to simplify tests.

mihaibudiu · 2022-08-18T21:25:09Z

Does this work with any version of cmake?
Is it faster or comparable to the old unified build?

fruffy · 2022-08-19T07:04:45Z

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.

mihaibudiu · 2022-08-19T17:32:32Z

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.

fruffy · 2022-08-20T12:38:27Z

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.

jfingerh · 2022-08-20T14:59:21Z

"We dropped support for anything lower than Ubuntu 20.04 with p4lang/third-party#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.

jfingerh · 2022-08-20T15:03:28Z

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.

fruffy · 2022-08-21T11:48:40Z

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 #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.

davidbolvansky · 2022-12-11T15:41:21Z

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.

fruffy · 2022-12-11T17:48:26Z

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.

jfingerh · 2022-12-11T22:21:58Z

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.

davidbolvansky · 2022-12-13T01:00:14Z

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?

fruffy · 2023-02-17T15:48:29Z

@mbudiu-vmw @vhavel This PR needs authorized approval because it touches some Testgen code.

mihaibudiu · 2023-02-17T17:48:50Z

CI and cmake are not my speciality, so I'd rather have someone else comment.

fruffy · 2023-02-17T18:20:20Z

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.

fruffy · 2023-02-21T14:19:19Z

@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.

frontends/common/parser_options.cpp

vlstill · 2023-02-21T15:06:05Z

.github/workflows/ci-validation.yml

@@ -35,8 +35,15 @@ jobs:

    - name: Build (Ubuntu 20.04)
      run: |
-        tools/ci-build.sh
+        tools/ci-build.sh || echo "BUILD_SUCCESS=false" >> $GITHUB_ENV


Could this be non-required tests instead? Some already are optional -- it was possible to merge PRs even with failing macOS test when it was broken recently. Is there some other status than OK/failure the build can be set to. I'm afraid this will lead to situations when the failure is ignored for a long time.

Fix build. Missing build_unified. Clean up cmakelists. Fix build flags. Need to include GENERATED property. Wrap clashing variable names in appropriate namespace. Cpplint. Simplify CI. Aggressive batch size. Fix template. cpplint. Build fixes. Fixup.

mihaibudiu

This looks fine, but I really don't know whether the new requirements will prevent people with older software from building the compiler.

mihaibudiu · 2023-02-21T21:01:38Z

backends/p4tools/modules/testgen/targets/bmv2/backend/stf/stf.cpp

@@ -235,7 +235,7 @@ inja::json::array_t STF::getClone(const std::map<cstring, const TestObject *> &c
    return cloneJson;
 }

-static std::string getTestCase() {
+std::string STF::getTestCaseTemplate() {


how are these changes related to unified builds?

When multiple of the test .cpp files are compiled together this function violates the one-definition rule and the compiler complains.

fruffy · 2023-02-21T21:12:58Z

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.

fruffy · 2023-02-23T15:37:34Z

@mbudiu-vmw @vlstill and @davidbolvansky both approved but I can not merge this PR since it is missing an authoritative approval.

jnfoster · 2023-02-23T15:38:49Z

I believe I have the power. Done.

fruffy · 2023-02-23T15:40:14Z

Thank you!

fruffy force-pushed the remove_unified_build branch 2 times, most recently from f22d878 to b5832b7 Compare August 18, 2022 12:19

fruffy marked this pull request as ready for review August 18, 2022 12:24

fruffy force-pushed the remove_unified_build branch from 759ae44 to 9ea33cf Compare August 18, 2022 16:09

fruffy requested a review from mihaibudiu August 18, 2022 21:22

fruffy-bfn mentioned this pull request Aug 30, 2022

Contribute the P4Tools platform and P4Testgen #3495

Merged

fruffy force-pushed the remove_unified_build branch 2 times, most recently from c2ea44a to efdd767 Compare October 21, 2022 18:10

fruffy requested review from pkotikal and jnfoster as code owners October 21, 2022 18:54

fruffy force-pushed the remove_unified_build branch from f01e64b to 2287822 Compare October 21, 2022 22:59

fruffy mentioned this pull request Oct 24, 2022

CI: added build with clang #3607

Merged

fruffy force-pushed the remove_unified_build branch 3 times, most recently from dd6b3da to a2c0e0a Compare October 28, 2022 17:20

pkotikal requested a review from ChrisDodd November 22, 2022 05:56

fruffy force-pushed the remove_unified_build branch from a2c0e0a to 48dbe1c Compare December 14, 2022 20:29

fruffy requested a review from vhavel as a code owner December 14, 2022 20:29

fruffy force-pushed the remove_unified_build branch from 81fbf19 to f42c1f4 Compare February 17, 2023 13:16

davidbolvansky approved these changes Feb 17, 2023

View reviewed changes

fruffy requested a review from vlstill February 21, 2023 14:17

vlstill reviewed Feb 21, 2023

View reviewed changes

fruffy added 8 commits February 21, 2023 15:56

Remove the unified build.

3c88259

Fix build. Missing build_unified. Clean up cmakelists. Fix build flags. Need to include GENERATED property. Wrap clashing variable names in appropriate namespace. Cpplint. Simplify CI. Aggressive batch size. Fix template. cpplint. Build fixes. Fixup.

Legacy options for unified compilation.

f63dd67

Warning.

a716473

Versioning.

47da77d

Install CMake 3.16.3 for Ubuntu 18.04.

4d947a8

Cosmetic edits.

44e8b98

Do not use generic namespaces when boost is present.

94cf84f

Do not ignore Gauntlet build issues.

d5734d7

fruffy force-pushed the remove_unified_build branch from e5b6782 to d5734d7 Compare February 21, 2023 20:56

mihaibudiu reviewed Feb 21, 2023

View reviewed changes

fruffy force-pushed the remove_unified_build branch from a65fc38 to 6feb7ce Compare February 22, 2023 23:15

Rename p4runtime functions because of conflict gmock macro.

d7a2c55

fruffy force-pushed the remove_unified_build branch from 6feb7ce to d7a2c55 Compare February 23, 2023 13:57

vlstill approved these changes Feb 23, 2023

View reviewed changes

fruffy requested a review from mihaibudiu February 23, 2023 15:37

jnfoster approved these changes Feb 23, 2023

View reviewed changes

fruffy merged commit ca1f347 into p4lang:main Feb 23, 2023

fruffy deleted the remove_unified_build branch February 23, 2023 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate unified build in favor of unity build. #3491

Deprecate unified build in favor of unity build. #3491

fruffy commented Aug 18, 2022 •

edited

Loading

fruffy commented Aug 18, 2022

mihaibudiu commented Aug 18, 2022

fruffy commented Aug 19, 2022

mihaibudiu commented Aug 19, 2022

fruffy commented Aug 20, 2022 •

edited

Loading

jfingerh commented Aug 20, 2022

jfingerh commented Aug 20, 2022

fruffy commented Aug 21, 2022

davidbolvansky commented Dec 11, 2022

fruffy commented Dec 11, 2022

jfingerh commented Dec 11, 2022

davidbolvansky commented Dec 13, 2022

fruffy commented Feb 17, 2023

mihaibudiu commented Feb 17, 2023

fruffy commented Feb 17, 2023 •

edited

Loading

fruffy commented Feb 21, 2023

vlstill Feb 21, 2023

mihaibudiu left a comment

mihaibudiu Feb 21, 2023

fruffy Feb 21, 2023 •

edited

Loading

fruffy commented Feb 21, 2023

fruffy commented Feb 23, 2023

jnfoster commented Feb 23, 2023

fruffy commented Feb 23, 2023

Deprecate unified build in favor of unity build. #3491

Deprecate unified build in favor of unity build. #3491

Conversation

fruffy commented Aug 18, 2022 • edited Loading

fruffy commented Aug 18, 2022

mihaibudiu commented Aug 18, 2022

fruffy commented Aug 19, 2022

mihaibudiu commented Aug 19, 2022

fruffy commented Aug 20, 2022 • edited Loading

jfingerh commented Aug 20, 2022

jfingerh commented Aug 20, 2022

fruffy commented Aug 21, 2022

davidbolvansky commented Dec 11, 2022

fruffy commented Dec 11, 2022

jfingerh commented Dec 11, 2022

davidbolvansky commented Dec 13, 2022

fruffy commented Feb 17, 2023

mihaibudiu commented Feb 17, 2023

fruffy commented Feb 17, 2023 • edited Loading

fruffy commented Feb 21, 2023

vlstill Feb 21, 2023

Choose a reason for hiding this comment

mihaibudiu left a comment

Choose a reason for hiding this comment

mihaibudiu Feb 21, 2023

Choose a reason for hiding this comment

fruffy Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

fruffy commented Feb 21, 2023

fruffy commented Feb 23, 2023

jnfoster commented Feb 23, 2023

fruffy commented Feb 23, 2023

fruffy commented Aug 18, 2022 •

edited

Loading

fruffy commented Aug 20, 2022 •

edited

Loading

fruffy commented Feb 17, 2023 •

edited

Loading

fruffy Feb 21, 2023 •

edited

Loading