Skip to content

Lazy configuration#156

Merged
cortinico merged 4 commits intogetsentry:mainfrom
remcomokveld:fix/eager-configuration-test
Aug 3, 2021
Merged

Lazy configuration#156
cortinico merged 4 commits intogetsentry:mainfrom
remcomokveld:fix/eager-configuration-test

Conversation

@remcomokveld
Copy link
Copy Markdown
Contributor

@remcomokveld remcomokveld commented Jul 30, 2021 •
edited
Loading

📜 Description

Changes the Plugin to use TaskProviders instead of tasks directly to ensure that tasks from AGP are not configured eagerly.

💡 Motivation and Context

Lazy Configuration "can avoid resource intensive work during the configuration phase, which can have a large impact on build performance".

💚 How did you test it?

The newly added test verifies that no additional tasks are being configured when invoking the help task.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

@remcomokveld remcomokveld force-pushed the fix/eager-configuration-test branch from cab1381 to db4126e Compare July 30, 2021 07:14
@marandaneto
Copy link
Copy Markdown
Contributor

marandaneto commented Jul 30, 2021

@remcomokveld that's great, thanks for that.

indeed, the usage of project.findTask(x) makes those tasks be triggered eagerly, but not all tasks have a TaskProvider<Task> so we'll have to find out how to properly setup task dependencies without it, we'll have a look into it, in case you have any ideas, that would be awesome, thanks.

@cortinico
Copy link
Copy Markdown
Contributor

cortinico commented Jul 30, 2021

but not all tasks have a TaskProvider<Task> so we'll have to find out how to properly setup task dependencies without it

Yup that's basically the problem :/ Hopefully AGP will migrate more and more to have TaskProvider for the tasks we depend on.

@remcomokveld
Copy link
Copy Markdown
Contributor Author

remcomokveld commented Jul 30, 2021

It is not AGP that define task providers, it is a Gradle feature, and for every task that has been registered you can obtain a TaskProvider using tasks.named().

Just pushed changes that does that, and now the test passes.

@remcomokveld remcomokveld requested a review from cortinico July 30, 2021 09:12
@remcomokveld remcomokveld force-pushed the fix/eager-configuration-test branch from 38befea to d921ba3 Compare July 30, 2021 09:15
@remcomokveld remcomokveld changed the title Lazy configuration test Lazy configuration Jul 30, 2021
@cortinico
Copy link
Copy Markdown
Contributor

cortinico commented Jul 31, 2021

and for every task that has been registered you can obtain a TaskProvider using tasks.named().

TIL :)

Thanks for addressing this 👍 Great stuff!

@cortinico
Copy link
Copy Markdown
Contributor

cortinico commented Aug 2, 2021

@marandaneto Are you fine with merging this?

@marandaneto
Copy link
Copy Markdown
Contributor

marandaneto commented Aug 2, 2021

@marandaneto Are you fine with merging this?

I'm OOO till next Monday,but by what I've seen on my mobile, its LGTM, please merge it :)

@cortinico cortinico merged commit 8a6528e into getsentry:main Aug 3, 2021
@remcomokveld remcomokveld deleted the fix/eager-configuration-test branch August 3, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marandaneto marandaneto Awaiting requested review from marandaneto

1 more reviewer

@cortinico cortinico cortinico 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.

3 participants

@remcomokveld @marandaneto @cortinico