Skip to content

Warn for overrides that are not having any effect#17822

Closed
AbrilRBS wants to merge 2 commits intoconan-io:develop2from
AbrilRBS:ar/warn-empty-overrides
Closed

Warn for overrides that are not having any effect#17822
AbrilRBS wants to merge 2 commits intoconan-io:develop2from
AbrilRBS:ar/warn-empty-overrides

Conversation

@AbrilRBS
Copy link
Copy Markdown
Member

@AbrilRBS AbrilRBS commented Feb 20, 2025

Changelog: Feature: Warn for overrides that are not having any effect
Docs: Omit

This warns when you have an override that has no affect upstream. This would lead to unexpeceted issues:

  • If you meant force=True, there's now a missing direct requirement and trying to build will most likely result in a missing dependency in your build system
  • If this was truly just an override, what's the use-case here? An optional dependency of your dependencies that your current configuration does not trigger?

Added to 2.22, but might not be moved forward if we are not comfortable with the possible false positives

@AbrilRBS AbrilRBS requested a review from memsharded February 20, 2025 20:01
Copy link
Copy Markdown
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.

As the broken tests reflect, this change is more risky than it appears.
It would be ok if something could be done just by inspecting the results, but no functional changes.

@AbrilRBS AbrilRBS force-pushed the ar/warn-empty-overrides branch from f68ce8a to fe059bb Compare July 8, 2025 18:45
@AbrilRBS AbrilRBS requested a review from memsharded July 8, 2025 18:45
@AbrilRBS
Copy link
Copy Markdown
Member Author

AbrilRBS commented Jul 8, 2025

Please re-recheck if this approach would be better

Copy link
Copy Markdown
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.

Yes, this approach looks better and safer.
And it should return any false positives, I think?

@AbrilRBS AbrilRBS changed the title Changes needed for override warn Warn for overrides that are not having any effect Oct 6, 2025
@AbrilRBS AbrilRBS added this to the 2.22.0 milestone Oct 6, 2025
@AbrilRBS
Copy link
Copy Markdown
Member Author

AbrilRBS commented Oct 6, 2025

And it should return any false positives, I think?

Everything I come up with is, maybe there's an optional dependency of your dependencies that your current configuration does not trigger, but that you want to override its version in the configurations that do pull it in? That's a valid use-case, and the more I think about it, the more I think then this should not warn there, and as there's no way to detect that false-positive, we might want to withdraw this PR. Let me know what you think @memsharded

@AbrilRBS AbrilRBS marked this pull request as ready for review October 6, 2025 23:54
@memsharded
Copy link
Copy Markdown
Member

Everything I come up with is, maybe there's an optional dependency of your dependencies that your current configuration does not trigger, but that you want to override its version in the configurations that do pull it in? That's a valid use-case, and the more I think about it, the more I think then this should not warn there, and as there's no way to detect that false-positive, we might want to withdraw this PR. Let me know what you think @memsharded

I really appreciate the value of warning about useless overrides, but yes, even if not frequent, such false positives could be a problem, so probably not worth the extra complexity and false positive risks.

@AbrilRBS AbrilRBS closed this Oct 7, 2025
@czoido czoido removed this from the 2.22.0 milestone Oct 28, 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