Skip to content

refactor: Revamp CI workflows#5661

Merged
bthomee merged 70 commits intodevelopfrom
bthomee/ci
Aug 18, 2025
Merged

refactor: Revamp CI workflows#5661
bthomee merged 70 commits intodevelopfrom
bthomee/ci

Conversation

@bthomee
Copy link
Collaborator

@bthomee bthomee commented Aug 10, 2025

High Level Overview of Change

This PR refactors the CI workflows to leverage the new CI Docker images for Debian, Red Hat, and Ubuntu. All workflows and actions have been rewritten as follows.

On each PR commit

The following jobs run:

  • Check format: same functionality as current CI for clang-format, adds additional formatting using prettier.
  • Check levelization: same functionality as current CI, but scripts have been moved into the .github directory.
  • Build and Test:
    • Linux: Runs Debian, Red Hat, and Ubuntu in various configurations. Some configurations are used to test specific operations, such as setting the reference fee to a different value, and codecov.
    • MacOS.
    • Windows.
    • Before building and testing, any needed dependencies are pulled from the Conan remote, and any missing ones are installed. In PR commits these dependencies are not uploaded.
  • Notify Clio: similar functionality as current CI, except that we now use the needs: keyword to wait on the Linux, MacOS, and Windows jobs instead of using a third-party action to poll for the completion of only the Linux jobs.

On trigger

The following jobs run for the following events:

  • Schedule: Each work day the dependencies are fully rebuilt and any missing ones are uploaded.
    • This is just as a sanity check, and is similar to how Clio does this.
    • In normal circumstances the rebuilt dependencies should be exactly the same as what is available in the Conan remote. However, if a Conan recipe received a patch in our Conan Center fork, it is possible for the rebuilt dependency to be different. Also, if the Conan remote has received maintenance and some packages were deleted, this ensures they are restored.
  • Workflow dispatch: This is triggered manually using the GitHub UI or API.
    • It supports two "force" flags, to rebuild all dependencies and to upload (=overwrite) the dependencies.
  • Push: When a PR is merged into the develop, release, or master branch the Build and Test workflows are run, as described in the section above, but this time any missing dependencies are uploaded.
    • This ensures that when a dependency is updated it will be added to the Conan remote upon merge of an approved PR into the develop branch.
    • The uploading is also enabled when merging into the release and master branch, since in rare cases a hotfix may result in a dependency change.

Context of Change

The CI workflows needed some TLC after the Conan 2 update and the new availability of our Conan Center Index fork.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

Future Tasks

It can be considered to add extra labels that, when applied to a PR, will trigger a specific run using one or more specific configurations. For instance, if a configuration using Clang 19 fails to build we may only notice this during merge; being able to trigger this during a PR will facilitate debugging.

@codecov
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (develop@ceb0ce5). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             develop   #5661   +/-   ##
=========================================
  Coverage           ?   78.7%           
=========================================
  Files              ?     814           
  Lines              ?   71639           
  Branches           ?    8443           
=========================================
  Hits               ?   56357           
  Misses             ?   15282           
  Partials           ?       0           

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bthomee bthomee requested a review from Bronek August 12, 2025 09:18
@bthomee
Copy link
Collaborator Author

bthomee commented Aug 15, 2025

I'll merge this after 2.6.0 has been released to not cause any disruptions during the release process.

@bthomee
Copy link
Collaborator Author

bthomee commented Aug 17, 2025

I made some small tweaks to further improve the workflows, and I think they're now in a good place.

  • The on-pr.yml workflow now uses on: pull_request.
    • That event has proven to be quite fragile, but it's now working.
    • I removed the on: push as this caused duplicate workflow invocations.
    • I removed the branches from the on: pull_request as we still want the workflows to run for other base branches too, e.g. the smart escrows branch or when you're working on a chain of dependent PRs.
    • I improved the logic for when the workflow should be triggered. In short, you no longer need to close and reopen a PR to get it to trigger, but can simply remove and re-add the DraftCIRun label as the logic now also triggers on the labeled and unlabeled events - but intentionally only for the DraftCIRun label and not for other labels.
  • Simplified the strategy matrix script to take an --all flag instead of having to work out itself based on the branch whether to generate all configurations or a subset.

As for follow-ups:

  • Simplify the strategy matrix generation to remove the .json files and handle it wholly within the .py file using the cross-product of all possible combinations and then filtering them down to the actually viable ones.
  • Consider adding support for triggering configurations to be built for PR commits that are otherwise excluded.
    • For instance, we know that the RHEL builds currently fail on ARM64, so it would be nice to be able to add a label or some other mechanism to explicitly trigger them on PR commits to test whether a proposed fix works.
  • Add support for asan, tsan, ubsan like Clio.
    • This requires some attention, as they affect the dependencies too. We should be fine running them in a Linux PR though, since the changes are isolated between runs and dependencies are not uploaded.

parser = argparse.ArgumentParser()
parser.add_argument('-c', '--config', help='Path to the JSON file containing the strategy matrix configuration.')
parser.add_argument('-r', '--ref', help='Git reference to generate the strategy matrix for (e.g. /refs/heads/develop).')
parser.add_argument('-a', '--all', help='Set to generate all configurations (generally used when merging a PR) or leave unset to generate a subset of configurations (generally used when committing to a PR).', action="store_true")
Copy link
Collaborator

@Bronek Bronek Aug 18, 2025

Choose a reason for hiding this comment

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

nice 👍

conan_remote_username: ${{ secrets.CONAN_REMOTE_USERNAME }}
conan_remote_password: ${{ secrets.CONAN_REMOTE_PASSWORD }}

build-windows:
Copy link
Collaborator

@mathbunnyru mathbunnyru Aug 18, 2025

Choose a reason for hiding this comment

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

These 3 build-<OS> would look much better as a matrix, because the only thing which changes is os.
And when you unite them you'll also be able to move dependencies_force_build and dependencies_force_upload to just one place, so there will be no need to set it in generate-outputs.

This workflow will become much shorter and understandable.

And the same thing is true about on-pr workflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, great points as follow-ups. I've got a list of things we can further improve.

@bthomee bthomee merged commit dc1caa4 into develop Aug 18, 2025
47 of 48 checks passed
@bthomee bthomee deleted the bthomee/ci branch August 18, 2025 14:21
ximinez added a commit that referenced this pull request Aug 18, 2025
…actoring-1

* XRPLF/develop:
  fix: Modify jobs to use '>>' instead of 'tee' for GITHUB_OUTPUT (#5699)
  refactor: Revamp CI workflows (#5661)
  refactor: Decouple net from xrpld and move rpc-related classes to the rpc folder (#5477)
  Set version to 2.6.0-rc2
  docs: Updates list of maintainers and reviewers (#5687)
  fix: Change log to debug level for AMM offer retrieval and IOU payment check (#5686)
  fix: Add -Wno-deprecated-declarations for Clang only (#5680)
  Update .git-blame-ignore-revs for #5657 (#5675)
  Fix BUILD.md instruction (#5676)
  Set version to 2.6.0-rc1
  fix: Improve logging of the reason to refuse a peer connection (#5664)
  fix: Make test suite names match the directory name (#5597)
  chore: Run prettier on all files (#5657)
  chore: Set CONAN_REMOTE_URL also for forks (#5662)
  chore: Cleanup bin/ directory (#5660)
  perf: Optimize hash performance by avoiding allocating hash state object (#5469)
@mvadari mvadari mentioned this pull request Aug 18, 2025
9 tasks
ximinez added a commit that referenced this pull request Aug 18, 2025
…to ximinez/lending-XLS-66

* XRPLF/ximinez/lending-refactoring-4:
  fix: Modify jobs to use '>>' instead of 'tee' for GITHUB_OUTPUT (#5699)
  refactor: Revamp CI workflows (#5661)
  refactor: Decouple net from xrpld and move rpc-related classes to the rpc folder (#5477)
  Set version to 2.6.0-rc2
  docs: Updates list of maintainers and reviewers (#5687)
  fix: Change log to debug level for AMM offer retrieval and IOU payment check (#5686)
  fix: Add -Wno-deprecated-declarations for Clang only (#5680)
  Update .git-blame-ignore-revs for #5657 (#5675)
  Fix BUILD.md instruction (#5676)
  Set version to 2.6.0-rc1
  fix: Improve logging of the reason to refuse a peer connection (#5664)
  fix: Make test suite names match the directory name (#5597)
  chore: Run prettier on all files (#5657)
  chore: Set CONAN_REMOTE_URL also for forks (#5662)
  chore: Cleanup bin/ directory (#5660)
  perf: Optimize hash performance by avoiding allocating hash state object (#5469)
yinyiqian1 pushed a commit to yinyiqian1/rippled that referenced this pull request Aug 20, 2025
This change refactors the CI workflows to leverage the new CI Docker images for Debian, Red Hat, and Ubuntu.
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