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

Set -Xjvm-default=all-compatibility compiler arg #178

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

szabado-faire
Copy link
Contributor

@szabado-faire szabado-faire commented Apr 3, 2024

I think I finally cracked #171. It turns out the issue is kotlin related.

It seems the root of the problem was that our project has the -Xjvm-default=all compiler flag set. Commenting it out eliminates this issue.

Seems like there's some subtle backwards incompatibility around annotations on pre-Java 8 interfaces with default implementations. Before Java 8, interfaces didn't support default implementations and would have a DefaultImpls static class declared to emulate that behaviour. Kotlin defaults to that pre-Java 8 behaviour (docs), and generates DefaultImpl objects. It seems like tempest using the pre-java-8 style here causes in projects that enable the post-java 8 behaviour.

all-compatibility seems like the correct setting to apply to solve this, based on these docs:

In addition to the all mode, generate compatibility stubs in the DefaultImpls classes. Compatibility stubs could be useful for library and runtime authors to keep backward binary compatibility for existing clients compiled against previous library versions. all and all-compatibility modes are changing the library ABI surface that clients will use after the recompilation of the library. In that sense, clients might be incompatible with previous library versions. This usually means that you need a proper library versioning, for example, major version increase in SemVer.

Hopefully this will fix my side without breaking other users of tempest.

@szabado-faire
Copy link
Contributor Author

An alternative is just updating to all. That'll only break any users of tempest that are on Java 7; aws has dropped support for Java 6

Java 7 has entered EOL and no longer gets security updates, so it's probably fine to drop support for

@szabado-faire szabado-faire changed the title Set -Xjvm-default=all compiler arg Set -Xjvm-default=all-compatibility compiler arg Apr 3, 2024
@kyeotic
Copy link
Collaborator

kyeotic commented Apr 3, 2024

Increasing compatibility on the library is probably the right move, but as the note says

This usually means that you need a proper library versioning, for example, major version increase in SemVer.

Unfortunately tempest does not use SemVer, so the only way to release a major version is to create a tempest3 version. This would also require that we introduce separate build steps for each version.

Or we could release as-is and be ready to rollback... I hate our versioning strategy. I would much rather be on SemVer.

@szabado-faire
Copy link
Contributor Author

Or we could release as-is and be ready to rollback...

I think this is probably the easiest path forwards here. If it breaks something we can roll back

Copy link
Collaborator

@kyeotic kyeotic left a comment

Choose a reason for hiding this comment

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

I hope this works

@kyeotic kyeotic merged commit 899f19f into cashapp:main Apr 3, 2024
2 checks passed
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.

2 participants