Skip to content

Apply legacy plugin last, and declare capabilities for old plugins#991

Merged
Goooler merged 6 commits intoGradleUp:mainfrom
jpenilla:legacy-plugin-application-order
Oct 24, 2024
Merged

Apply legacy plugin last, and declare capabilities for old plugins#991
Goooler merged 6 commits intoGradleUp:mainfrom
jpenilla:legacy-plugin-application-order

Conversation

@jpenilla
Copy link
Copy Markdown
Contributor

@jpenilla jpenilla commented Sep 29, 2024 •
edited
Loading

fixes #964 using the solution suggested by @ljacomet at #964 (comment)

Tested to fix the issue (bring behavior back to how it was before 8.3.1/with the old plugin), and to give an error when both the current and old plugin are on the classpath.

Something else that came to mind regarding the legacy plugin is that if someone puts the new plugin on the classpath, but then applies the legacy plugin id, nothing will happen, which could be confusing. But resolving that is likely better addressed in a separate PR (if at all).

> Could not resolve all files for configuration ':build-logic:compileClasspath'.
   > Could not resolve com.gradleup.shadow:shadow-gradle-plugin:8.3.3-SNAPSHOT.
     Required by:
         project :build-logic
      > Module 'com.gradleup.shadow:shadow-gradle-plugin' has been rejected:
           Cannot select module with conflict on capability 'com.github.johnrengelman:shadow:8.3.3-SNAPSHOT' also provided by [com.github.johnrengelman:shadow:8.1.1(apiElements)]
   > Could not resolve com.github.johnrengelman:shadow:8.1.1.
     Required by:
         project :build-logic
      > Module 'com.github.johnrengelman:shadow' has been rejected:
           Cannot select module with conflict on capability 'com.github.johnrengelman:shadow:8.1.1' also provided by [com.gradleup.shadow:shadow-gradle-plugin:8.3.3-SNAPSHOT(apiElements)]
  • CHANGELOG's "Unreleased" section has been updated, if applicable.

Goooler
Goooler approved these changes Sep 29, 2024
View reviewed changes
@Goooler
Copy link
Copy Markdown
Member

Goooler commented Sep 29, 2024

CC @ljacomet

ljacomet
ljacomet approved these changes Oct 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ljacomet ljacomet left a comment

Choose a reason for hiding this comment

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

Sorry, I said I would attempt this and in the end did not find the time.

This LGTM!

The one thing I wondered was about the capability addition: should it be done in a patch release or in a minor?
My preference would be to do it in a minor as it might end up breaking builds (for good reasons) while the fix to the order of application fits perfectly a patch release.

@ljacomet
Copy link
Copy Markdown
Contributor

ljacomet commented Oct 24, 2024

And thank you @jpenilla for taking care of fixing my mistake!

@Goooler
Copy link
Copy Markdown
Member

Goooler commented Oct 24, 2024

I'm planning to release this in 8.3.4, WDYT?

@Goooler Goooler enabled auto-merge (squash) October 24, 2024 08:31
@Goooler Goooler merged commit 8aad9ad into GradleUp:main Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Goooler Goooler Goooler approved these changes

+1 more reviewer

@ljacomet ljacomet ljacomet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Legacy plugin application order breaks shadow detection logic updated for new id

3 participants

@jpenilla @Goooler @ljacomet