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

Start making each FormatterStep roundtrip serializable. #1945

Merged
merged 27 commits into from
Jan 23, 2024

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Dec 4, 2023

I don't like the design of the Gradle configuration cache. It puts too much burden on plugin developers, and it's too slow for end users (#987). But our workaround is a bit of a hassle, and we've had a really active contributor pool lately so maybe we have the brainpower to do the big rewrite that it would take to remove the JvmLocalCache workaround and fully support roundtrip serialization.

This PR introduces StepHarnessBase, which is a mechanism for enforcing that every individual formatter step must support roundtrip serialization. It will take a while for every step to support it, so we can adjust this StepHarnessBase as more and more steps add support. Once every step supports it, we will enforce this requirement for all new steps.

This PR also introduces FormatterStepEqualityOnStateSerialization. This allows us to keep the exact same equality semantics we have used so far, but it gives us more flexibility to implement serialization. Especially serialization of absolute paths without screwing up buildcache keys.

…e can start testing round trip serialization on steps.
@nedtwigg
Copy link
Member Author

nedtwigg commented Dec 5, 2023

@Goooler does this approach make sense? A nice thing is that we could merge these bit by bit, we don't have a hard fork situation, and once we have completed all of these we can remove JvmLocalCache and we'll have full native support for Gradle's configuration cache.

@jbduncan
Copy link
Member

jbduncan commented Dec 6, 2023

This PR looks fantastic! Let me know when you're close to the end and I'd be more than happy to give this a proper review.

@nedtwigg nedtwigg requested a review from jbduncan December 6, 2023 20:53
@nedtwigg
Copy link
Member Author

nedtwigg commented Dec 6, 2023

@jbduncan and @Goooler, a code review would be very welcome if you have time, thanks! In this PR I did

  • the infrastructure
  • applied the infrastructure to a simple step (Indent) and a typical step (Diktat).

I have also used this approach for some of our harder steps (eclipse), and I'm about to try the PipeStepPair too, which is one of the harder ones I think.

@jbduncan
Copy link
Member

@nedtwigg It took me all evening to get my machine set up for Spotless again (fixing my vastly outdated fork on GitHub which was still using master, for instance), so I've not had a chance to review the code yet. Maybe next weekend!

@Goooler
Copy link
Member

Goooler commented Dec 11, 2023

Use gh pr checkout 1945.

@jbduncan
Copy link
Member

Thanks for the pointer, @Goooler! I've already fixed my fork and run git checkout serializable-refactor, but I'll consider gh for the future. 👍

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

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

Looking good, and I like the incremental approach to the new roundtrip serialization mechanism. A few things from me (see the review comments), but otherwise a fantastic start!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@nedtwigg
Copy link
Member Author

Thanks very much for this thorough review! My plan is

  • deploy main as-is (in progress)
  • merge this and its follow-ons
  • definitely allow breaking changes in lib
  • hoping no breaking changes in plugin-maven or plugin-gradle, but TBD

Until we have complete switched over to configuration cache, I'd rather not publish any releases. That creates a bit of an awkward phase, but I think we can keep it down to just one or two weeks. Can always backtrack and make some kind of bugfix release branch if we have to.

@nedtwigg nedtwigg enabled auto-merge January 23, 2024 23:02
@nedtwigg nedtwigg merged commit 26fbd03 into main Jan 23, 2024
19 checks passed
@nedtwigg nedtwigg deleted the serializable-refactor branch January 23, 2024 23:19
@jbduncan
Copy link
Member

LGTM! 👍

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.

3 participants