-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Switch Conan 1 commands to Conan 2 and fix credentials #5655
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
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically this discussion:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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 🚀 New features to boost your workflow:
|
There was a problem hiding this 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 👍
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
.gitignore
, formatting, dropping support for older tooling)