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

Simplify Plugins approach to adding dependencies so that it no longer uses a hidden detached configuration. #345

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

arlaneenalra
Copy link
Contributor

What's changed?

Removed dependency copying out of the rewrite configuration to a hidden detached one.

What's your motivation?

We have internal tooling that relies on being able to attach to configurations with a beforeResolve hook override dependencies and provide centralized dependency versioning. With the detached configuration there is not way to attach the hook and dependency resolution fails. By removing the detached configuration in favor of the rewrite configuration we should be able to use normal hooks as expected.

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

We've attempted several approaches without modifying the existing plugin but each has run into subtle problems.

  • Resolving our internal dependencies first (currently a library of internal recipes) and injecting the resovled files as dependencies. - Lead to duplicated jars on the classpath.
  • Replacing the dependencies provider externally with one we can control. - There's no way to get a list of the dependencies the plugin adds without reflectively changing access levels, other forms of nastiness, or repeating the first problem but from the opposite side.

Any additional context

Checklist

  • [ ] I've added unit tests to cover both positive and negative cases Existing test cases already cover the behavior and the expected behavior should not change.
  • [/] I've read and applied the recipe conventions and best practices
  • [/] I've used the IntelliJ IDEA auto-formatter on affected files

@arlaneenalra
Copy link
Contributor Author

Just realized this is running knownRewriteDependencies too soon ... I'll need to fix that.

With the `addLater` approach that was previously attempted the
dependency strings were generated early. This would prevent
users from being able to set the rewrite version in their build
file. Using `beforeResolve` allows us to delay until just before
resolution.
@arlaneenalra
Copy link
Contributor Author

Ok, switched to beforeResolve instead of addLater That lombok issue is still present from the mainline. Not sure what to do with that.

@arlaneenalra
Copy link
Contributor Author

Ping, What are the next steps here?

@timtebeek timtebeek added the enhancement New feature or request label Nov 19, 2024
@shanman190
Copy link
Collaborator

shanman190 commented Nov 22, 2024

@arlaneenalra, right now it appears that the same failing test on main is causing you some grief here. Just to confirm, it is just the lombok test that fails with the testGradle4 task? If so, we can probably go ahead and get this merged and afterwards fix the test. I think Sam was planning to rework the lombok setup very soon, so this test would probably get updated due to that.

@arlaneenalra
Copy link
Contributor Author

@arlaneenalra, right now it appears that the same failing test on main is causing you some grief here. Just to confirm, it is just the lombok test that fails with the testGradle4 task? If so, we can probably go ahead and get this merged and afterwards fix the test. I think Sam was planning to rework the lombok setup very soon, so this test would probably get updated due to that.

I'm pretty sure that particular test is actually skipped when using testGradle4. Yeah I just verified, it get's skipped for that task.


RewriteRunTest > lombokTypeAttribution(File) SKIPPED

Probably because of

It is the only test to fail for the generic test and the ci build from what I can tell.

@shanman190 shanman190 merged commit 147c7ef into openrewrite:main Nov 22, 2024
1 check failed
@timtebeek
Copy link
Contributor

hi @arlaneenalra ; thanks again! Your change is now available through
https://github.com/openrewrite/rewrite-gradle-plugin/releases/tag/v6.28.0

Hope that helps you there!

@arlaneenalra
Copy link
Contributor Author

arlaneenalra commented Nov 27, 2024 via email

@timtebeek timtebeek mentioned this pull request Nov 29, 2024
timtebeek added a commit that referenced this pull request Dec 9, 2024
…o longer uses a hidden detached configuration. (#345)"

This reverts commit 147c7ef.
timtebeek added a commit that referenced this pull request Dec 9, 2024
* Format RewritePluginTest.kt

* Revert "Simplify Plugins approach to adding dependencies so that it no longer uses a hidden detached configuration. (#345)"

This reverts commit 147c7ef.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants