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

Upload libXRPL flow #5028

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

godexsoft
Copy link
Collaborator

@godexsoft godexsoft commented May 24, 2024

High Level Overview of Change

Adding a workflow that automatically publishes libxrpl to Artifactory and triggers a workflow in Clio's CI using repository_dispatch.

This flow will only run if the BuildInfo.cpp file was updated in a PR for the release branch. Typically this means a new version is proposed.

Publishing the library as soon as possible (i.e. not after merging it or tagging the version) gives us the ability to notify Clio's CI and trigger a full check against the new version of libxrpl. In the future we could automate a notification back from Clio to report whether the checks were successful or not.

This flow will start together with others (if conditions are met) but will not proceed with the upload until essential other checks are done and are either successful or skipped (not cancelled or failed). Currently the essential checks are all dependencies and test running on linux. Mac and Windows are not on the list. Main reason for that is Mac being flaky and we don't want to fail this step (at least not often).

Two secrets that need to be present in the repo (are already present):

  • CONAN_USERNAME
  • CONAN_TOKEN

And a new one that needs to be added in order to trigger a workflow on Clio's CI:

  • CLIO_NOTIFY_TOKEN

Please contact me to get the token for this secret.

Here is the flow in Clio CI that does the rest: XRPLF/clio#1433

Context of Change

Recently there was an rc of libxrpl that did not allow consumers to link correctly. We should try and avoid this in the future.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)

API Impact

None.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.1%. Comparing base (40b4adc) to head (f7c169d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5028     +/-   ##
=========================================
- Coverage     71.1%   71.1%   -0.0%     
=========================================
  Files          796     796             
  Lines        66997   66997             
  Branches     10981   10980      -1     
=========================================
- Hits         47638   47634      -4     
- Misses       19359   19363      +4     

see 4 files with indirect coverage changes

Impacted file tree graph

@godexsoft godexsoft force-pushed the feature/ci-publish-libxrpl1 branch from 9fed9c9 to f7c169d Compare May 28, 2024 20:45
@godexsoft godexsoft marked this pull request as ready for review May 28, 2024 21:25
paths:
- 'src/ripple/protocol/impl/BuildInfo.cpp'
- '.github/workflows/libxrpl.yml'
branches: [release]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Here is a patch release PR that targeted master.
  • Here is a major release PR that targeted develop.

Do we channel all of our releases through a single branch? I don't know. Even if we do, I'm not sure we can count on it always happening through a PR. I understand you want to publish a package before the commit is pushed to a protected branch or tagged. Can we get an expert on the release process to chime in? @seelabs?

Once we determine the right condition to check, we can think about how to deduplicate some of the steps here. Maybe we can move this workflow into a single conditional step at the end of the test 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.

Would be indeed useful to know if release is supposed to always be used for this purpose @seelabs 👍

When implementing this i deliberately sacrificed duplication for non-intrusiveness. I agree we can deduplicate quite a bit but i thought we would be better off creating a separate issue/PR for that. Let me know what you think @thejohnfreeman @seelabs .

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've added a workflow in a branch that builds the Conan package locally and tests that an example downstream package can import it successfully. We could add a job to upload the recipe (and binary?) conditional on that test (and whatever conditions we decide determine a release).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how useful it is to build an "example" package. Unless you have a package that uses everything libxrpl provides of course, in which case that's great.
I only really care that Clio (the only real/important consumer afaik) still works so I'd certainly prefer to leave the notification step that triggers Clio's CI. If you want to add an example package build in between - i have no problem with that.
I personally prefer to get simple things like CI out of the way and functional quicker and then improve if needed later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is useful to test that, at least, libxrpl can be found and linked by a dependent package. It is possible to build a Conan package that doesn't actually export anything that can be linked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Clio could be our example package, instead, if we want to make it so. But the test that Clio can build against the xrpl Conan package must be moved to this project, so that it is a blocker for our commits. The example package I wrote is much simpler, but it is in this project. That is the key attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, i personally don't like the idea of building Clio from this repo - that's as intrusive as it gets. But i do see your points. Please do what you want with this PR or let's discuss next steps next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seelabs can we get your input on the release process please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@thejohnfreeman @godexsoft:

You can rely on releases and release candidates always being pushed to release. In most cases, you can rely on betas being pushed to release as well, but that may not happen if there are develop has diverged from release and we aren't merging betas into release until we transition the release candidate to release.

Here's the process that happens when develop and master have not diverged (usually the branches have not diverged, but this can happen for hot fixes or when a rc is active and we merge commits to develop that shouldn't be in the rc):

  • betas and release candidates are merged to develop and release
  • releases are merged to develop, release and master

I understand that happened with the old process when develop and master diverged, but it's less clear to me what happens in the new process. Here's my understanding for the new process if release and develop have diverged:

  • betas to to develop, but not master
  • release candidates to to release but not develop
  • once a release candidate transitions to a release, we merge the release branch into develop with a merge commit.
  • once the reverse happens, new betas will be merged to master again until the branches diverge

We usually open beta PRs and rc PRs against the release branch, but sometimes the PR is accidentally opened against the develop branch and if the branches haven't diverged there's no harm.

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.

None yet

4 participants