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

Enable [python-infer].unowned_dependency_behavior="warning" by default. #16281

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Jul 23, 2022

Rough consensus was reached on #15326 and #14975 that enabling unowned_dependency_behavior="warning" by default would:

  1. be helpful for new users, to guide them through fixing their missing dependencies
  2. be useful in the case of adding additional resolves (due to the work done in Warn when dependency inference does not infer something in your resolve #15326 to enrich the warning for that case)
  3. reduce the chances of CI errors in --changed may not detect an inferred dependency on a deleted (or malformed) file #14975 (when users go a step further to make the warning an error)

2.14.x is a relatively quiet release so far, so it seems like there is room in the budget for a new warning like this.

The rendered message for this warning is very self-explanatory due to @thejcannon's original work, and @Eric-Arellano's followup work in #15326, so I don't see any obvious documentation changes that need to be made.

[ci skip-build-wheels]
[ci skip-rust]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I'm good with this, but wait for Eric plz

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Technicallyyyy this violates our deprecation policy. But I'm comfortable with it because it is a warning, not an error. Your build will still work when upgrading to 2.14, only with possible warnings

@benjyw
Copy link
Contributor

benjyw commented Jul 25, 2022

I don't think this is a deprecation violation? We make no promise about warnings.

@Eric-Arellano
Copy link
Contributor

I don't think this is a deprecation violation?

Changing the default value of an existing option is a deprecation violation. But I think this is For The Greater Good, and the important thing is we're not breaking your builds. Only adding warnings.

@Eric-Arellano
Copy link
Contributor

The safer alternative is to deprecate not setting a default, then in the subsequent release, change the default. But that has downsides like brand-new users being hit with a deprecation warning.

@stuhood stuhood merged commit 897a8aa into pantsbuild:main Jul 26, 2022
@stuhood stuhood deleted the stuhood/enable-unowned-dependency-behavior-warn branch July 26, 2022 15:02
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.

4 participants