-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add flag to control native maven jar rule deprecation #6768
Conversation
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.
Please also document the flag at site/docs/skylark/backward-compatibility.md
@@ -476,6 +476,20 @@ | |||
+ "will be available") | |||
public boolean incompatibleRemoveNativeHttpArchive; | |||
|
|||
@Option( | |||
name = "incompatible_remove_native_maven_jar", | |||
defaultValue = "true", |
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.
AFAIK new flags should be added with the default value "false", this way people will be able to check whether their code is compatible with all the upcoming incompatible changes in advance.
new EvalException( | ||
null, | ||
"The native maven_jar rule is deprecated." | ||
+ " add details about replacement here." |
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 line looks like a TODO.
Please follow Communicating Breaking Changes here (create flag with as off by default, file an issue tarcking migration) |
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.
LGTM modulo the existing comments.
Thanks for the review (everyone), I'm not sure why you all got added. I set the flag to true because I wanted to test against downstream dependencies. I'm going to address the changes. |
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.
LGTM for docs.
No description provided.