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

Backward compatibility with Okio 1.x #1677

Merged
merged 1 commit into from
Dec 18, 2019
Merged

Backward compatibility with Okio 1.x #1677

merged 1 commit into from
Dec 18, 2019

Conversation

pyricau
Copy link
Member

@pyricau pyricau commented Dec 17, 2019

LeakCanary was using APIs introduced in Okio 2.x (extension functions), which breaks any consumers that forces Okio to 1.x . This change reverts to using the 1.x APIs. Unfortunately we cannot directly compile against 2.x because the deprecated APIs have an Error deprecation level. So here the module that depends on Okio compiles against 1.x, and then its consumer module depends on 2.x so that the resolution defaults to the higher version. This has the advantage of enforcing that we write code that compiles against two versions of Okio

Fixes #1624

cc @swankjesse @JakeWharton in case you have some thoughts on this.

Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Force downgrading is not a safe operation and other libraries using Okio 2.x are liable to break. This library already depends on Kotlin so downgrading Okio provides no advantage. You're of course welcome to proceed with this since it doesn't affect much. I would push back on the user either way.

Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

Personally I’d build against either 1.x or 2.x, but not both. But this looks safe.

@@ -40,7 +39,7 @@ class Hprof private constructor(
if (currentPosition == newPosition) {
return
}
source.buffer.clear()
source.buffer().clear()
Copy link
Member

Choose a reason for hiding this comment

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

You should use the latest 1.x release. In that you'll see this is deprecated; replaced with getBuffer().

Copy link
Member

Choose a reason for hiding this comment

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

Version 1.17.4.

Copy link
Member

Choose a reason for hiding this comment

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

Oh and in great irony the bug report is from a user on a version that precedes this one. That is, this bug can occur exclusively within 1.x releases. My recommendation is to decline this PR and tell your user to use the latest 1.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

buffer vs getBuffer will work for 1.17.4, but I expect the extension functions weren't available then.

@pyricau
Copy link
Member Author

pyricau commented Dec 17, 2019

Thanks! I'll keep this out for a bit, trying to gather feedback on why people are actively forcing to 1.x . I'll make the call once I have more info.

LeakCanary was using APIs introduced in Okio 2.x (extension functions), which breaks any consumers that forces Okio to 1.x . This change reverts to using the 1.x APIs. Unfortunately we cannot directly compile against 2.x because the deprecated APIs have an Error deprecation level. So here the module that depends on Okio compiles against 1.x, and then its consumer module depends on 2.x so that the resolution defaults to the higher version. This has the advantage of enforcing that we write code that compiles against two versions of Okio

Fixes #1624
@pyricau
Copy link
Member Author

pyricau commented Dec 18, 2019

Merging this, as I believe this will not change much from public API standpoint (LeakCanary still depends on 2.x) and only changes what its been compiled against. While this wouldn't be a sustainable approach if we had a large number of dependencies, Okio is the only required dependency of LeakCanary. So this change effectively increases the range of apps that can use LeakCanary with no additional work.

If and when Okio drops support for these deprecated APIs, I'll upgrade LeakCanary and either drop support or find a way to provide dual support (similar to the dual support lib + Android X support)

@pyricau pyricau merged commit a42f078 into master Dec 18, 2019
@pyricau pyricau deleted the py/okio_1x branch December 18, 2019 11:01
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.

ava.lang.NoSuchMethodError: No interface method getBuffer()Lokio/Buffer
3 participants