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

Fix #561 - cleanup task input/output issues #567

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DPUkyle
Copy link
Member

@DPUkyle DPUkyle commented Mar 23, 2021

  • Consumers of the pegasus plugin must use Gradle 5.4+
    • @ReplacedBy annotation requires Gradle 5.4+
    • o.g.w.InputChanges API requires Gradle 5.4+

--

  • Instead of using @ReplacedBy, another possibility could be to make malformed property accessor methods throw UnsupportedOperationException
  • Blocks Bump Gradle wrapper to 6.1 #566, which will bump the wrapper to 6.1. Unlike this change which forces consumers to use Gradle 5.4 or higher,
    consumers of the plugin will not be forced to use Gradle 6.1.
  • The removed API, o.g.a.t.i.IncrementalTaskInputs, is deprecated starting from Gradle 6.0 and fails compilation as this project fails its own build on compiler warnings.

An alternative to this PR would be to:

  1. remove the methods annotated with @ReplacedBy, as they have been in a deprecated state for nearly two years, and
  2. allow compiler warnings but only for the gradle-plugins subproject.

Even with these steps, however, a Gradle 6.1+ upgrade is needed soon. I plan to reach out to the team members to discuss.

 - Consumers of the pegasus plugin must use Gradle 5.4+
   - `@ReplacedBy` annotation requires Gradle 5.4+
   - `o.g.w.InputChanges` API requires Gradle 5.4+
@DPUkyle DPUkyle marked this pull request as ready for review March 23, 2021 14:17
@DPUkyle DPUkyle linked an issue Mar 23, 2021 that may be closed by this pull request
Copy link
Contributor

@aman1309 aman1309 left a comment

Choose a reason for hiding this comment

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

is there a company wide initiative to move everyone to gradle>5.4, IMO only removing some warnings by using @Replacedby doesn't fit as a good use-case to do an incompatible change. Maybe we merge this with 6.1 upgrade if and when we need it. wdyt?

@DPUkyle
Copy link
Member Author

DPUkyle commented Mar 30, 2021

is there a company wide initiative to move everyone to gradle>5.4, IMO only removing some warnings by using @Replacedby doesn't fit as a good use-case to do an incompatible change. Maybe we merge this with 6.1 upgrade if and when we need it. wdyt?

Gradle 7.0 will be out soon and I plan to aggressively encourage upgrading to Gradle 5.6.4 at a minimum.

Nevertheless, as I mentioned above I could implement an alternative approach not requiring @ReplacedBy and leaving o.g.a.t.i.IncrementalTaskInputs in place for now:

An alternative to this PR would be to:

  1. remove the methods annotated with @ReplacedBy, as they have been in a deprecated state for nearly two years, and
  2. allow compiler warnings but only for the gradle-plugins subproject.

This makes the changes backward-compatible and buys some time, but at some point this plugin will have to use APIs which won't support older versions of Gradle.

@aman1309
Copy link
Contributor

aman1309 commented Mar 30, 2021

This makes the changes backward-compatible and buys some time, but at some point this plugin will have to use APIs which won't support older versions of Gradle.

I would prefer deferring until we need to or there is some feature we would like to add available only in later versions because the cost of migrating consumers is high and we should have a greater incentive.

@DPUkyle
Copy link
Member Author

DPUkyle commented Mar 30, 2021

This makes the changes backward-compatible and buys some time, but at some point this plugin will have to use APIs which won't support older versions of Gradle.

I would prefer deferring until we need to or there is some feature we would like to add available only in later versions because the cost of migrating consumers is high and we should have a greater incentive.

Sounds fine. I will refactor to remove non-backwards-compatible changes.

@DPUkyle DPUkyle marked this pull request as draft April 2, 2021 04:14
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.

Gradle tasks missing annotations
2 participants