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

Fix #247 ensure binary compatibility from now #248

Merged
merged 4 commits into from
Oct 28, 2021
Merged

Conversation

ohze
Copy link

@ohze ohze commented Oct 4, 2021

Background

FunctionConverters is generated by an internal fnGen sbt project that use scala-compiler & scala-reflect to generate converter for all interfaces in java.util.function package.

Problem

fnGen is not reliable. It produce different code on different run [1]
In #211, @SethTisue have to add some ProblemFilters.exclude.
But, as describing in #247, the mima Problem should not be filtered out because it will cause real binary incompatible error in runtime.

Solution by this PR

  1. Remove mima filters
  2. Reimplement fnGen: Don't use scala-compiler & scala-reflect

The generated code by the new fnGen implementation is exactly same as the generated code by the old fnGen on my machine. (somehow, the old fnGen is reliable in my machine :D)
You can and should verify that in your machine too.


[1]

  • When run sbt sources on CI (on my fork) and check the generated code, I found that the output is different in some CI jobs / runs.
  • Version 0.9.1, 1.0.0 and 1.0.1 released on maven central have different FunctionConverters not only between versions but also in the same version with different scala cross build (ex .._2.12-1.0.1.jar vs .._2.12-1.0.1.jar). See this CI job that run mima against all released version from 0.9.1 and only v0.9.1 for scala 2.12 is mima check passed.

@SethTisue SethTisue requested a review from lrytz October 4, 2021 22:31
@ohze
Copy link
Author

ohze commented Oct 5, 2021

I have force-pushed just to update some comments

@SethTisue
Copy link
Member

@lrytz I haven't looked at the fnGen stuff before. but this certainly looks plausible, and we should get a release out before much longer, to correct the bincompat breakage. any objection to just merging this?

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Sorry @ohze that I missed the notifications of this PR.

Thank you very much for investigating this complicated problem and providing a solid fix. LGTM!

@SethTisue SethTisue merged commit 96fa643 into scala:main Oct 28, 2021
@SethTisue SethTisue mentioned this pull request Oct 28, 2021
6 tasks
gcf-merge-on-green bot referenced this pull request in googleapis/java-pubsublite-spark Nov 3, 2021
…1 to v1.0.2 (#295)

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [org.scala-lang.modules:scala-java8-compat_2.11](http://www.scala-lang.org/) ([source](https://github.com/scala/scala-java8-compat)) | `1.0.1` -> `1.0.2` | [![age](https://badges.renovateapi.com/packages/maven/org.scala-lang.modules:scala-java8-compat_2.11/1.0.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/org.scala-lang.modules:scala-java8-compat_2.11/1.0.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/org.scala-lang.modules:scala-java8-compat_2.11/1.0.2/compatibility-slim/1.0.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/org.scala-lang.modules:scala-java8-compat_2.11/1.0.2/confidence-slim/1.0.1)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>scala/scala-java8-compat</summary>

### [`v1.0.2`](https://github.com/scala/scala-java8-compat/releases/v1.0.2)

[Compare Source](https://github.com/scala/scala-java8-compat/compare/v1.0.1...v1.0.2)

#### What's Changed

-   Fix [#&#8203;247](https://github.com/scala/scala-java8-compat/issues/247) ensure binary compatibility from now by [@&#8203;ohze](https://github.com/ohze) in [https://github.com/scala/scala-java8-compat/pull/248](https://github.com/scala/scala-java8-compat/pull/248)

#### New Contributors

-   [@&#8203;ohze](https://github.com/ohze) made their first contribution in [https://github.com/scala/scala-java8-compat/pull/248](https://github.com/scala/scala-java8-compat/pull/248)

**Full Changelog**: scala/scala-java8-compat@v1.0.1...v1.0.2

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-pubsublite-spark).
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.

4 participants