Skip to content

Add tools-rippled-clang-format#5

Merged
bthomee merged 9 commits intomainfrom
Bronek/add-tools-rippled
Jul 7, 2025
Merged

Add tools-rippled-clang-format#5
bthomee merged 9 commits intomainfrom
Bronek/add-tools-rippled

Conversation

@Bronek
Copy link
Collaborator

@Bronek Bronek commented Jul 2, 2025

Aside from build images, XRPLF projects also use container images with specialized tools, e.g. to enforce code formatting, run sanitizers, run code coverage tools etc. The required images are created by workflows starting with tools- and ending with the project name e.g. tools-rippled, and are only meant to support specific projects. These images may also contain a complete C++ build environment, if needed.

This change adds the image for clang-format.

@Bronek Bronek force-pushed the Bronek/add-tools-rippled branch from 6424b18 to 1ffd170 Compare July 2, 2025 15:48
@bthomee
Copy link
Contributor

bthomee commented Jul 2, 2025

  • With the tools image, are there packages that can be removed from the Debian, RHEL, and Ubuntu images?
  • The tools image for rippled contains a few packages that had been added for Clio (e.g. bison, flex) that can probably be removed here.

@Bronek
Copy link
Collaborator Author

Bronek commented Jul 2, 2025

With the tools image, are there packages that can be removed from the Debian, RHEL, and Ubuntu images?

I think we can get rid of gcovr when this is merged; I would keep it for now in #4 , so we have a sequence of changes rather than making assumptions about not-yet-ready PR.

The tools image for rippled contains a few packages that had been added for Clio (e.g. bison, flex) that can probably be removed here.

As long as they are not needed to actually build clio, or run its unit tests, or package it, they can be moved to tools-clio (which this PR could also include, after change of the subject line of course).

@bthomee
Copy link
Contributor

bthomee commented Jul 2, 2025

The tools image for rippled contains a few packages that had been added for Clio (e.g. bison, flex) that can probably be removed here.

As long as they are not needed to actually build clio, or run its unit tests, or package it, they can be moved to tools-clio (which this PR could also include, after change of the subject line of course).

@mathbunnyru which of the tools that I added to the base images are not needed to build Clio?

@mathbunnyru
Copy link
Collaborator

The tools image for rippled contains a few packages that had been added for Clio (e.g. bison, flex) that can probably be removed here.

As long as they are not needed to actually build clio, or run its unit tests, or package it, they can be moved to tools-clio (which this PR could also include, after change of the subject line of course).

@mathbunnyru which of the tools that I added to the base images are not needed to build Clio?

I don't think we need bison, curl, flex, wget (but I'm ok keeping curl and wget to make images easier to use)

@Bronek Bronek force-pushed the Bronek/add-tools-rippled branch from 8f3a096 to 7e31c4f Compare July 4, 2025 15:03
@Bronek Bronek marked this pull request as ready for review July 4, 2025 15:03
@Bronek Bronek requested review from bthomee and legleux July 4, 2025 15:03
Copy link
Contributor

@bthomee bthomee left a comment

Choose a reason for hiding this comment

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

Since the idea is to make separate tools-clio-X images containing the tools they need, such as bison and flex, should we just remove them already from all three dockerfiles in this PR?

Separately, I know that to enable coverage analysis it is needed to enable -Dcoverage=ON when building. Is it really more efficient to trigger a separate job for just coverage that will need to build from scratch (I don't think we yet have a good way of caching intermediate build artifacts?), as opposed to just running it as part of the regular build+test for just one of the matrix combinations?

For example, in the rippled CI we'd then eventually have something like:

matrix:
  mode:
    - Debug
    - Release
  os:
      - image: jammy:gcc-12
         platform: linux/amd64
         runner: ubuntu-24.04
      - image: jammy:gcc-12
         platform: linux/arm64
         runner: ubuntu-24.04-arm
         [...]

And in the CI pipeline you just activate the coverage option & upload the report when mode == 'Debug' && image == 'noble:gcc-14' && platform == 'linux/amd64'. Yes, Gcovr would be part of images where it would never be used, but it would avoid launching a completely separate job for just coverage that would use extra compute.

Similarly, setting -Dvoidstar=ON could be done similarly. Would could enable coverage and instrumentation at the same time.

@Bronek
Copy link
Collaborator Author

Bronek commented Jul 7, 2025

Since the idea is to make separate tools-clio-X images containing the tools they need, such as bison and flex, should we just remove them already from all three dockerfiles in this PR?

Yes I guess, in the appropriate PR which creates tools-clio-X - which is not this PR.

Separately, I know that to enable coverage analysis it is needed to enable -Dcoverage=ON when building. Is it really more efficient to trigger a separate job for just coverage that will need to build from scratch (I don't think we yet have a good way of caching intermediate build artifacts?), as opposed to just running it as part of the regular build+test for just one of the matrix combinations?

That is not such a good idea - unit tests run slower when coverage is enabled.

And in the CI pipeline you just activate the coverage option & upload the report when mode == 'Debug' && image == 'noble:gcc-14' && platform == 'linux/amd64'. Yes, Gcovr would be part of images where it would never be used, but it would avoid launching a completely separate job for just coverage that would use extra compute.

I guess that's a good idea, but such change does not belong here and also, -Dcoverage=ON would have to be subject to the same switch (as not to slow down all unit tests, as explained above).

Similarly, setting -Dvoidstar=ON could be done similarly. Would could enable coverage and instrumentation at the same time.

Can use a similar (but not the same) switch for instrumentation check, but note that we can only use this option with Clang compiler. Also, it might similarly slow down unit tests, just a little.

For now do not rename the image; it makes more sense after rippled have switched to pre-commit
@Bronek Bronek force-pushed the Bronek/add-tools-rippled branch from af94ea0 to d1e1b29 Compare July 7, 2025 13:47
@Bronek Bronek requested a review from bthomee July 7, 2025 13:53
@bthomee bthomee changed the title Add tools-rippled Add tools-rippled-clang-format Jul 7, 2025
@bthomee bthomee merged commit 0f944f3 into main Jul 7, 2025
3 checks passed
@bthomee bthomee deleted the Bronek/add-tools-rippled branch July 7, 2025 19:24
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.

3 participants