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

Delegate error handling to command framework instead of graph formatters #16415

Merged
merged 13 commits into from
Jul 31, 2024

Conversation

perseoGI
Copy link
Contributor

@perseoGI perseoGI commented Jun 6, 2024

Changelog: Bugfix: Make conan graph a real "install" dry-run. Return same errors and give same messages
Docs: omit

Close #16393

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@perseoGI perseoGI added this to the 2.5.0 milestone Jun 6, 2024
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking good 👍

return install_order_serialized

return {"build_order": install_order_serialized,
"conan_error": result.get_errors()}
Copy link
Member

Choose a reason for hiding this comment

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

Is this even possible? How can a build-order-merge have errors?

perseoGI added 2 commits July 1, 2024 12:51
Make conan graph <subcommand> a real "install" dry-run. Return same
errors and give same messages
@perseoGI perseoGI force-pushed the 16393-consistent-graph-command branch from 1c4a009 to d89b5b9 Compare July 1, 2024 13:24
conan/cli/command.py Outdated Show resolved Hide resolved
conan/cli/command.py Outdated Show resolved Hide resolved
conan/cli/command.py Outdated Show resolved Hide resolved
conan/cli/command.py Outdated Show resolved Hide resolved
conan/cli/command.py Outdated Show resolved Hide resolved
conan/cli/commands/graph.py Show resolved Hide resolved
conans/client/graph/install_graph.py Show resolved Hide resolved
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking better, getting closer! :)

conans/client/graph/install_graph.py Outdated Show resolved Hide resolved
Comment on lines 480 to 485
def get_errors(self):
try:
self.raise_errors(basic_output=True)
except ConanException as e:
return str(e)
return None
Copy link
Member

Choose a reason for hiding this comment

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

It looks like that instead of modifying the _raise_missing and doing this, it would be better to craft its own simpler message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But raise_errors could potentially raise invalid binaries errors also.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sure, it can iterate all packages and add an error message line for each invalid and missing.

@memsharded memsharded modified the milestones: 2.5.0, 2.6.0 Jul 2, 2024
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Good, it can be moved forward

conans/client/graph/install_graph.py Outdated Show resolved Hide resolved
@perseoGI perseoGI marked this pull request as ready for review July 3, 2024 15:36
@memsharded memsharded requested a review from czoido July 31, 2024 07:58
conan/cli/command.py Outdated Show resolved Hide resolved
@memsharded memsharded self-assigned this Jul 31, 2024
@memsharded memsharded merged commit 592399b into conan-io:develop2 Jul 31, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Consistent command results and error messages accross install/create and graph
4 participants