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

Use a constant for JvmOverloads to avoid a crash due to relocation #4008

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Apr 8, 2022

A fix for #4004.

There were also a few other instances or classes used directly, which didn't seem to cause trouble, but also fixed here for consistency.

@netlify
Copy link

netlify bot commented Apr 8, 2022

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit d95ae6f
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/6253fab496faa90008536faf

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

👍 I think the JDK classes (like java.lang.Override and java.lang.String) are not relocated because they're not part of the fat jar so that'd explain why this has worked so far.

Doesn't hurt to not use them reflectively though 👍

@martinbonnin
Copy link
Contributor

CI fails with

fullstack/LaunchDetailsQuery.kt: (22, 19):  Declaration annotated with '@OptionalExpectation' can only be used in common module sources

@BoD
Copy link
Contributor Author

BoD commented Apr 8, 2022

Nothing's ever easy is it? 😅
I'm guessing this is due to the specific wiring in integration-tests to have both Java and Kotlin codegens?

I moved the test to a specific submodule instead - JVM only but also tried locally on a MP one which also didn't have the issue.

@martinbonnin
Copy link
Contributor

I'm guessing this is due to the specific wiring in integration-tests to have both Java and Kotlin codegens?

How would that be an issue? Java codegen is not impacted and Kotlin should compile in all cases?

@BoD
Copy link
Contributor Author

BoD commented Apr 8, 2022

How would that be an issue? Java codegen is not impacted and Kotlin should compile in all cases?

That was kind of a wild guess, but in fact I just noticed it's compileTestKotlinJs that's failing 🤔. Will investigate more.

@BoD
Copy link
Contributor Author

BoD commented Apr 8, 2022

I can repro Declaration annotated with '@OptionalExpectation' can only be used in common module sources by setting this on my test project:

  outputDirConnection {
    connectToKotlinSourceSet("jsTest")
    connectToKotlinSourceSet("jvmTest")
    connectToKotlinSourceSet("appleTest")
  }

but to be honest I'm still bewildered

@@ -177,12 +176,13 @@ class Options(
val generateOptionalOperationVariables: Boolean = defaultGenerateOptionalOperationVariables,

/**
* Whether to generate kotlin constructors with @JvmOverloads for more graceful Java interop experience when default values are present.
* Whether to generate kotlin constructors with `@JvmOverloads` for more graceful Java interop experience when default values are present.
* Note: when enabled in a multi-platform setup, the generated code can only be used in the common or JVM sourcesets.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@martinbonnin martinbonnin merged commit 0c06bb3 into main Apr 11, 2022
@martinbonnin martinbonnin deleted the fix-jvm-overloads-relocation branch April 11, 2022 11:56
martinbonnin pushed a commit that referenced this pull request Apr 11, 2022
…4008)

* Use a constant for JvmOverloads to avoid a crash due to relocation

* Move test to own subproject

* Add a note about `@JvmOverloads` being legal only in common or JVM targets
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