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

Sort the type names in the list so the code gen is deterministic. #4138

Merged
merged 4 commits into from
May 27, 2022

Conversation

eboudrant
Copy link
Contributor

Fix for #4137

@netlify
Copy link

netlify bot commented May 25, 2022

👷 Deploy request for apollo-android-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 600772f

@martinbonnin
Copy link
Contributor

Thanks for sending this! Out of curiosity, are you seeing discrepancies only in Schema.kt?
I was expecting mutableMapOf<>() to have stable iteration order so that the codegen would be stable too. If it's not the case we might have issues in other places too....

@eboudrant
Copy link
Contributor Author

I only looked at Schema.kt for the moment, we could have more problems. I'd like to test it on my branch, what's the best way to build the plugin locally?

@martinbonnin
Copy link
Contributor

There's some info in the CONTRIBUTING.md. TLDR; ./gradlew publishToMavenLocal

@martinbonnin
Copy link
Contributor

The CI failed (expectedly) because the compiler tests output changed. You can update them with ./gradlew :apollo-compiler:test -DupadteTestFixtures=true

But it's still weird that we haven't seen any issue like this in the repo. Is there anything in your CI setup that could explain the instability? Java version, gradle parameters, anything else?

@eboudrant
Copy link
Contributor Author

We're looking if there was a change in CI. Not at my knowledge but we've seen similar issue with Autovalue Gson code gen.

@eboudrant
Copy link
Contributor Author

@martinbonnin should I also update the class com.apollographql.apollo3.compiler.codegen.java.file.SchemaBuilder?

@martinbonnin
Copy link
Contributor

should I also update the class com.apollographql.apollo3.compiler.codegen.java.file.SchemaBuilder

That'd be nice, if the bug exists with Kotlin codegen there's a high chance it is in the Java codegen too. Also keeps the both codegen symmetrical so 👍

@@ -240,6 +240,7 @@ kotlin-responseBased-subscriptions
kotlin-responseBased-enums_as_sealed 340
kotlin-responseBased-operation_id_generator 325
kotlin-responseBased-merged_include 323
kotlin-responseBased-big_query 317
kotlin-responseBased-enum_field 316
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why this line was added?

Copy link
Contributor

Choose a reason for hiding this comment

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

2 PRs were merged in parallel:

So in the end 4132 was merged without the new test that you just added. So thank you :)

FWIW, this file is informative only. It's used to keep track of the size of codegen. It's handy to have it commited because it allows to easily bisect if there is a regression but we might as well put it in .gitignore

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

👍

@BoD BoD merged commit 35dff0f into apollographql:main May 27, 2022
@eboudrant eboudrant deleted the deterministic_schema_kt branch May 27, 2022 15:18
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.

None yet

3 participants