Skip to content

Plugin is application ordering sensitive #309

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

Closed
Vampire opened this issue May 24, 2024 · 1 comment
Closed

Plugin is application ordering sensitive #309

Vampire opened this issue May 24, 2024 · 1 comment
Assignees
Milestone

Comments

@Vampire
Copy link
Contributor

Vampire commented May 24, 2024

The plugin is sensitive to application order.
This is discouraged bad practice and should be avoided.

Currently, the plugin checks in its apply method project.getPlugins().hasPlugin('com.android.application') and uses a different report task class accordingly.

Using project.getPlugins() per-se is bad already (see its JavaDoc).

But even if using the recommended alternative project.getPluginManager().hasPlugin(...), this would not fix the ordering sensitivity.

This only works as intended when the Android Application Plugin is applied before the License Report Plugin.
And also only if they are applied to the same project.

I don't know why you only make the task cacheable if the Android Application Plugin is not applied too, but if there is a substantial reason, it might also cause problems when applying the License Report Plugin to the root project and the Android Application Plugin to some subproject?

But even if the problem with cacheability really is only there if the two plugins are applied to the same project, the @Classpath input should most probably always be there, not only when the task is cacheable, or you might for example miss some implicit task dependencies that would have been necessary.

So I would suggest to

  • move the @Classpath property to the ReportTask,
  • declare the ReportTask as @CacheableTask,
  • delete the CacheableReportTask,
  • if the task really must not be cacheable if the Android Application Plugin is applied to the same project, then just use the runtime api like
    project.getPluginManager().withPlugin('com.android.application') {
        generateLicenseReportTask {
            cacheIf { false }
        }
    }
    This is better practice and is no longer application ordering sensitive.
@jk1
Copy link
Owner

jk1 commented May 26, 2024

Hi @Vampire! Thank you for bringing this up. It was actually lazy me back then who disabled caching for android projects in #119 instead of fixing it. So I believe that the ordering issue can be solved in a way you suggested.

The project.getPlugins().hasPlugin('com.android.application') clause is also used to determine configuration default, but this is something I can also work around.

@jk1 jk1 self-assigned this May 26, 2024
jk1 added a commit that referenced this issue May 26, 2024
jk1 added a commit that referenced this issue May 26, 2024
@jk1 jk1 added this to the 2.8 milestone May 26, 2024
@jk1 jk1 closed this as completed May 26, 2024
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

No branches or pull requests

2 participants