Skip to content

Conversation

bthomee
Copy link
Collaborator

@bthomee bthomee commented Aug 7, 2025

High Level Overview of Change

This change updates some incorrect Conan commands for Conan 2. This change further uses the org-level variables and secrets rather than the repo-level ones.

Context of Change

Some of the Conan commands were still for 1.x. This change updates them to Conan 2.x. As some flags do not exist in Conan 2, such as --settings build_type=[configuration], the commands have been adjusted accordingly.

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

@bthomee bthomee marked this pull request as draft August 7, 2025 13:15
@bthomee bthomee marked this pull request as ready for review August 7, 2025 17:23
@bthomee bthomee requested review from Bronek and mathbunnyru August 7, 2025 17:35
Comment on lines 41 to 43
echo "Uploading missing dependencies."
conan list --graph=build.json --graph-binaries=build --format=json > pkgs.json
conan upload --list=pkgs.json --confirm --check --remote xrplf
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be a part of dependencies action, but in a separate 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.

See earlier discussion. It's not going to be part of a separate workflow to avoid needless re-computation.

Copy link
Collaborator

@Bronek Bronek Aug 7, 2025

Choose a reason for hiding this comment

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

Specifically this discussion:

#5654 (comment)

And I agree with @bthomee that, given the number of platform combinations we need to build, it would be a huge PITA to have to do all the conan install on top of what we already do in the existing workflows. Even though I started the discussion from the opposite position.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't say we have to do conan install in a different place. I'm saying conan upload should be a separate workflow.
And it would avoid needless re-computation if configured properly, yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will look at how to move conan upload to a separate workflow/action in the CI refactor PR - I think it will be done sometime next week. For this PR I consider it out of scope - the intention of this PR was to primarily fix the authentication issue, and secondarily to address the concern that re-uploading unchanged packages might change the package hash.

Copy link
Collaborator

@Bronek Bronek Aug 8, 2025

Choose a reason for hiding this comment

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

I agree with @bthomee - let's try to keep the PR focused. Scope creep is an enemy of improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I think the current change is not focused already.
When the PR title is "do this and do that" in the title, it usually means it should be 2 PRs - "do this" and "do that"

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I think it would be better to keep this PR as "Switch Conan 1 commands to Conan 2" (more-or-less revert these lines) and then in the next PR discuss how, where and when we should upload packages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactoring of uploading packages to a separate workflow can go into a separate PR. It's important to keep refactoring changes away from functional ones. This PR is functional and does exactly what its subject line says.

Copy link

codecov bot commented Aug 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.8%. Comparing base (69314e6) to head (38685a3).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5655     +/-   ##
=========================================
- Coverage     78.8%   78.8%   -0.0%     
=========================================
  Files          814     814             
  Lines        71283   71243     -40     
  Branches      8349    8341      -8     
=========================================
- Hits         56175   56140     -35     
+ Misses       15108   15103      -5     

see 10 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Only two small nits, this aside I think it's good 👍

@Bronek Bronek requested a review from mathbunnyru August 8, 2025 11:07
@bthomee bthomee changed the title Switch Conan 1 commands to Conan 2 and upload only missing packages Switch Conan 1 commands to Conan 2 and fix credentials Aug 8, 2025
@bthomee bthomee enabled auto-merge (squash) August 8, 2025 12:21
@bthomee bthomee merged commit 39b5031 into develop Aug 8, 2025
29 checks passed
@bthomee bthomee deleted the bthomee/missing branch August 8, 2025 12:47
ximinez added a commit that referenced this pull request Aug 8, 2025
…actoring-1

* XRPLF/develop:
  Switch Conan 1 commands to Conan 2 and fix credentials (#5655)
  perf: Move mutex to the partition level (#5486)
  Upload Conan dependencies upon merge into develop (#5654)
This was referenced Aug 27, 2025
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