Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More C++ standard version change on build #4025

Merged
merged 4 commits into from
Mar 6, 2025
Merged

Conversation

zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Mar 6, 2025

  • Changes our public clang build to use C++23
  • Remove C++17 for DynamicType tests

If we migrate to C++20 today, a fair estimate is that we will migrate to C++23 in three years. Thanks to @xwang233's effort, we already have an internal clang build pipeline for C++20, so I am changing the public pipeline in GitHub actions to use C++23 so we start get signals. I would be curious to see the readiness of compilers for C++23, would it just work? Or would it fail often?

To be clear, I do not want to waste people's time to fight for compiler bugs. So if this C++23 build is failing due to compiler reasons, my expectation is people will just leave it in the failing state, instead of writing workarounds to make it work. On the other hand, C++20 -> C++23 is a much smaller upgrade compared to C++17 -> C++20, so I would expect it to be much smoother.

Over time, I would expect we will gradually have more and more pipelines running on C++23, if we are collecting good signals that compilers are stable enough. I see this as a three year process. The target date in my mind for formal switch to C++23 is the release date of Ubuntu 28.04 LTS, but in practice we can decide to do it earlier or later depending on the signal we get. Reference dates are: Ubuntu 22.04 LTS's standard support will end in Apr 2027, Ubuntu 24.04 LTS's standard support will end in Apr 2029. These reference date could be the date of change if we want to do it earlier or later.

Copy link

github-actions bot commented Mar 6, 2025

Review updated until commit ffdfe25

Description

  • Update build.yml to support C++23

  • Enable C++23 tests and benchmarks in CMakeLists.txt

  • Adjust meson.build to include C++23 tests


Changes walkthrough 📝

Relevant files
Enhancement
build.yml
Update build.yml for C++23                                                             

.github/workflows/build.yml

  • Rename clang-build job to clang-build-23
  • Add --cpp=23 flag to python setup.py build
  • +2/-2     
    CMakeLists.txt
    Enable C++23 in CMakeLists.txt                                                     

    lib/dynamic_type/CMakeLists.txt

  • Enable C++23 tests and benchmarks
  • Remove C++17 tests and benchmarks
  • +2/-4     
    meson.build
    Adjust meson.build for C++23                                                         

    lib/dynamic_type/meson.build

  • Adjust test and benchmark loops to include C++23
  • Remove C++17 from test and benchmark loops
  • +2/-2     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new job clang-build-23 and the updated python setup.py build --cpp=23 command assume that the build system supports C++23. This should be validated to ensure compatibility and that no regressions occur.

    clang-build-23:
      runs-on: ubuntu-latest
      steps:
        - uses: actions/checkout@v4
          with:
            submodules: true
        - uses: actions/setup-python@v4
          with:
            python-version: '3.10'
        - name: Build with clang++
          working-directory: ${{ env.working_directory }}
          run: |
            tools/apt-install-things.sh &
            tools/pip-install-things.sh &
            source tools/setup-env.sh
            wait
            python setup.py build --cpp=23
    Possible Issue

    The removal of add_test_for_standard(17) and the addition of add_test_for_standard(23) may lead to a gap in testing for C++17. This should be evaluated to ensure that the project's compatibility with C++17 is not compromised.

    add_test_for_standard(20)
    add_test_for_standard(23)
    
    Possible Issue

    The removal of foreach standard : ['17', '20', '23'] and the addition of foreach standard : ['20', '23'] may lead to a gap in testing for C++17. This should be evaluated to ensure that the project's compatibility with C++17 is not compromised.

    foreach standard : ['20', '23']
        name = 'test_dynamic_type_' + standard

    @zasdfgbnm zasdfgbnm marked this pull request as ready for review March 6, 2025 17:46
    @zasdfgbnm
    Copy link
    Collaborator Author

    !test

    @zasdfgbnm zasdfgbnm requested review from naoyam and xwang233 March 6, 2025 17:46
    @zasdfgbnm zasdfgbnm changed the title Update build.yml More C++ standard version change on build Mar 6, 2025
    Copy link
    Collaborator

    @naoyam naoyam left a comment

    Choose a reason for hiding this comment

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

    LGTM. Let's see if there's actually any bugs showing up.

    @zasdfgbnm zasdfgbnm merged commit 5111d3b into main Mar 6, 2025
    46 of 47 checks passed
    @zasdfgbnm zasdfgbnm deleted the zasdfgbnm-patch-5 branch March 6, 2025 18:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants