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

use source files as input for generating uuids and source bundle #634

Merged
merged 22 commits into from
Feb 5, 2024

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Jan 16, 2024

📜 Description

Reduce the amount of tasks that run on every build by adding input dependencies on the proguard mapping file and the source files. Also change some upToDateWhen rules to allow tasks to be considered UP-TO-DATE

💡 Motivation and Context

Fixes #616
Fixes #628
Fixes #285

💚 How did you test it?

Added automated tests to verify the behaviour.

📝 Checklist

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

🔮 Next steps

All tasks that use the sentry-cli are still run on every build. Most likely due to SentryCliValueSource.obtain returning a different value at each run, when the sentry-cli is unpacked from resources to a temp folder. This is something we should look into to improve build times and avoid unnecessary uploads to sentry.

Copy link
Contributor

github-actions bot commented Jan 16, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 2ad414c

@lbloder lbloder marked this pull request as ready for review February 1, 2024 13:15
@@ -33,6 +33,9 @@ abstract class UploadSourceBundleTask : Exec() {
includeSourceContext.getOrElse(false) &&
!sourceBundleDir.asFileTree.isEmpty
}

// Allows gradle to consider this task up-to-date if the inputs haven't changed
Copy link

Choose a reason for hiding this comment

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

👋 just a fly on the wall. I'm a sentry user and a stakeholder in issue #616, but I'm reasonably certain this is incorrect. See the JavaDoc for the upToDateWhen method: https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/TaskOutputs.html#upToDateWhen-groovy.lang.Closure-

Adds a predicate to determine whether previous outputs of this task can be reused. The given closure is executed at task execution time. The closure is passed the task as a parameter. If the closure returns false, previous outputs of this task cannot be reused and the task will be executed. That means the task is out-of-date and no outputs will be loaded from the build cache.

You can add multiple such predicates. The task outputs cannot be reused when any predicate returns false.

See this discussion: https://discuss.gradle.org/t/outputs-uptodatewhen-confusion/17973/3

If you read “false” as “not”, then…
outputs.upToDateWhen { true } means “outputs are up to date”.
outputs.upToDateWhen { false } means “outputs are not up to date”.

If any upToDateWhen spec returns false, the task is considered out of date. If they return true, Gradle does the normal behavior of checking input/output files.

This should be removed to avoid confusion, but I don't think it will negatively impact anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @ryandens,
Thanks for having a look.
This particular task does not have any outputs. Gradle does up-to-date checks based on a tasks inputs and outputs. If a task does not have any outputs it will never be considered up-to-date (found that in the gradle docs somewhere, but don't have it at hand right know).
Therefore I added the outputs.upToDateWhen closure to let gradle know that it should always consider the tasks output to be up-to-date and only run the task if its inputs change.

I will double-check though.

Hope this makes things a bit less confusing.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice one, this is great!

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, ideally we link the outputs.upToDateWhen { true } docs explanation from the comment as this will likely come up again in the future. Maybe add some negative up-to-date check tests (i.e. file changed -> rerun task).

@@ -55,6 +58,92 @@ class SentryPluginSourceContextTest :
assertTrue(result.output) { "BUILD SUCCESSFUL" in result.output }
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

l should we add more tests to check whether changing an input file actually reruns the task?

Copy link
Collaborator Author

@lbloder lbloder Feb 5, 2024

Choose a reason for hiding this comment

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

This would only make sense for taks SentryUploadProguardMappingsTask and UploadSourceBundleTask which are, at the moment always executed, due to the cliExecutable changing at every run (see NextSteps of the PR description). Once that issue is resolved, it would absolutely make sense to create such a test, especially to make sure the behaviour doesn't change in future gradle versions.

@adinauer adinauer self-assigned this Feb 5, 2024
@lbloder lbloder merged commit a4bd46d into main Feb 5, 2024
18 checks passed
@lbloder lbloder deleted the feat/source-context-build-cache branch February 5, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants