-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fixed https://github.com/google/gson/issues/1310 #1311
Conversation
/** | ||
* Utility to check the major Java version of the current JVM. | ||
*/ | ||
public class VersionUtils { |
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 is a public class in a non-internal
package. Renaming breaks binary compatibility.
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.
Ok, it is meant to be internal. I will move it out.
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.
It's not good that our process allowed this kind of mistake.
I propose some process changes:
- Code isn't merged without a code review. If the code review is stalled the change is stalled. Err on the side of not merging.
- Releases aren't released without a heads up to other maintainers. That gives us another opportunity to avoid mistakes.
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.
@swankjesse @JakeWharton Heads up to other maintainers is a good idea. How about a 2 day wait for response?
Code merging can't keep waiting for reviewers who are not responding. Let's set some SLA on it too.
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.
Let's plan a mailing list (or someother mechanism) that will alert all maintainers, and they will be expected to respond in a timely manner.
Given that this PR breaks (theoretical) binary compatibility, let's get the next release out ASAP. I plan to publish 2.8.5 with this change. Any concerns?
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.
I don't understand why this class was moved to create the binary incompatible change. Releasing this seems like a mistake.
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.
I made the mistake in Gson 2.8.4 in releasing a class in package com.google.gson.utils which seems like an official package. However, the term "official" is just arbitrary and applies to the package named "internal". Our intention was never to make VersionUtils public and likely no one has used it given that 2.8.4 has been out for a very short time.
In Gson 2.8.5, we will undo this mistake and take the risk of breaking code which happen to rely on that VersionUtil class. Likely there will be no such person. But we need to publish Gson 2.8.5 quickly, otherwise the chance of someone using that package keeps increasing.
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.
I see. I don't have a particularly strong opinion about it then, except that it seems to lend credibility to Jesse's suggestion of not merging anything or making releases without review. Given the scope of this project that's dangerous behavior.
I don't see any reason for an SLA. Gson doesn't need to be moving very fast.
@JakeWharton can you take another look? |
@JakeWharton merging. Please review and send me feedback, and I will incorporate it in another PR |
* Fixed google#1310 Also renamed VersionUtils to more readable abstraction JavaVersion Added support for debian naming convention Using min supported version (6) as the default if JDK version can't be figured out * Moved JavaVersion to an internal package
Also renamed VersionUtils to more readable abstraction JavaVersion
Added support for debian naming convention
Using min supported version (6) as the default if JDK version can't be figured out