Skip to content

PG-1467 Add clang builds to CI #230

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

Open
wants to merge 2 commits into
base: TDE_REL_17_STABLE
Choose a base branch
from

Conversation

artemgavrilov
Copy link
Collaborator

@artemgavrilov artemgavrilov commented Apr 17, 2025

@artemgavrilov artemgavrilov force-pushed the PG-1467-clang-ci branch 4 times, most recently from e05108e to 8d26e18 Compare April 23, 2025 16:31
@artemgavrilov artemgavrilov changed the title PG-1467 Add clang builds PG-1467 Add clang builds to CI Apr 23, 2025
@artemgavrilov artemgavrilov marked this pull request as ready for review April 23, 2025 16:45
Copy link
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

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

What feature from a more modern Meson did you need? That would be useful information to have in the commit message if someone sees this in the future and wonders why it is installed from pip.

@artemgavrilov
Copy link
Collaborator Author

What feature from a more modern Meson did you need? That would be useful information to have in the commit message if someone sees this in the future and wonders why it is installed from pip.

Old version of meson incorrectly checks existence of some libs/funcs/structs available in a system. That caused compilation failure due name collision. While update helped to solve original issue and compile project there are still differences between gcc and clang builds.

You can check and compare output of these two commands:
CC=gcc meson setup build --prefix build-gcc --buildtype=debug -Dcassert=true -Dtap_tests=enabled CC=clang meson setup build --prefix build-clang --buildtype=debug -Dcassert=true -Dtap_tests=enabled`

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.94%. Comparing base (fd43ead) to head (338e53f).

❌ Your project status has failed because the head coverage (78.94%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                Coverage Diff                 @@
##           TDE_REL_17_STABLE     #230   +/-   ##
==================================================
  Coverage              78.93%   78.94%           
==================================================
  Files                     22       22           
  Lines                   2464     2465    +1     
  Branches                 385      386    +1     
==================================================
+ Hits                    1945     1946    +1     
  Misses                   444      444           
  Partials                  75       75           
Components Coverage Δ
access 81.54% <ø> (ø)
catalog 86.32% <ø> (ø)
common 92.50% <ø> (ø)
encryption 71.90% <ø> (ø)
keyring 72.07% <ø> (ø)
src 55.73% <ø> (ø)
smgr 98.03% <ø> (+0.01%) ⬆️
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

artifact_name: build-${{ inputs.os }}-${{ inputs.build_script }}-${{ inputs.build_type }}
artifact_name: build-${{ inputs.os }}-${{ inputs.compiler }}-${{ inputs.build_script }}-${{ inputs.build_type }}
CC: ${{ inputs.compiler }}
CXX: ${{ inputs.compiler }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't correct, but maybe it works because we are only using C files. But it should be g++ or clang++

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You right, even though both compilers can automatically switch mode based on file extensions they still have differences against what stdlib program will be linked. However the only C++ code that we have is demos in libkmip and we don't compile it at all. So I will just drop CXX variable here.

Ubunut has outdated meson version in its repos. So intall it with pip
instead.
Add clang compiler to CI matrix
Copy link
Collaborator

@AndersAstrand AndersAstrand left a comment

Choose a reason for hiding this comment

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

Looks great!

Weirdly it seems to wait for results of ci runs that no longer exists?

@artemgavrilov
Copy link
Collaborator Author

Weirdly it seems to wait for results of ci runs that no longer exists?

They are marked as required in repo settings, so before merging it should be updated.

@AndersAstrand
Copy link
Collaborator

Weirdly it seems to wait for results of ci runs that no longer exists?

They are marked as required in repo settings, so before merging it should be updated.

Do we need to get IT helpdesk involved to do that or does anyone in the team have the proper access?

@artemgavrilov
Copy link
Collaborator Author

Weirdly it seems to wait for results of ci runs that no longer exists?

They are marked as required in repo settings, so before merging it should be updated.

Do we need to get IT helpdesk involved to do that or does anyone in the team have the proper access?

@dutow has admin permissions in the repo

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.

5 participants