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

Fix graph explain not showing some differences in requirements if missing #15355

Merged

Conversation

RubenRBS
Copy link
Member

@RubenRBS RubenRBS commented Dec 27, 2023

Changelog: Fix: Fix graph explain not showing some differences in requirements if missing.
Docs: Omit

Message when the right pkgid is not looked up still pending to add.
This still need to be rebased on top of #15353 for the tests to have a chance to pass, as it was while fixing this that we found the linked PR

This to discuss:

  • When one of the binaries has a conf that the other has not, it shows up in the missing one as being equal to None, instead of not showing up at all. Do we want this?
  • Tests. I wasn't able to test build-requires nor settings_target, a contribution for that would be welcome
  • General cleanup. There's some repetition, it might be nice to refactor some things out before merging

@RubenRBS RubenRBS marked this pull request as ready for review December 28, 2023 08:25
conan/api/subapi/list.py Outdated Show resolved Hide resolved
@RubenRBS RubenRBS marked this pull request as draft January 4, 2024 11:33
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.

Looks good to me, I think we can move it forward

Comment on lines 277 to 281
for r in expected_build_requires:
existing = binary_build_requires.get(r.name)
if not existing or r != existing:
self.build_requires_diff.setdefault("expected", set()).add(repr(r))
self.build_requires_diff.setdefault("existing", set()).add(repr(existing))
Copy link
Member

Choose a reason for hiding this comment

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

This pattern is repeated a few times, maybe we want to make a local function for it?

@memsharded memsharded marked this pull request as ready for review January 10, 2024 09:04
Copy link
Member Author

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Approved

@czoido czoido merged commit f08b992 into conan-io:develop2 Jan 10, 2024
2 checks passed
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

3 participants