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

Bug: caching issues for included builds #1220

Open
seregamorph opened this issue Jul 9, 2024 · 1 comment
Open

Bug: caching issues for included builds #1220

seregamorph opened this issue Jul 9, 2024 · 1 comment
Labels
bug Something isn't working performance
Milestone

Comments

@seregamorph
Copy link
Contributor

Problem statement

We've observed severe issues with our isolated (included) builds: the projectHealth producing invalid advices to remove a dependency (if applied as suggested - it just breaks the compilation). Worth to mention that problem only happened with build cache enabled (different behaviour with --no-build-cache executions).

Root cause analysis

Finally, after debugging and digging into build scans it was found that the problem is with these two tasks:

  • artifactsReportX (ArtifactsReportTask class)
  • graphViewX (GraphViewTask class)

and it's related to build caching. With equal (from the Gradle perspective) task inputs it is reusing inappropriate cached task output from another isolated builds.

The detailed analysis highlighted key differences in generated build/reports/dependency-analysis/{sourceSet}/intermediates/artifacts.json file (taken from cache vs no build cache):

  • coordinates.identifier which can be different depending on current root build project (prefix)
  • coordinates.resolvedProject.gradleVariantIdentification.capabilities - same - different prefix, depends on current root build project

Similar situation with GraphViewTask outputs: the graph-[compile/runtime].json output files contain incorrect included_build.identifier from another included build cached result. Or incorrect node.identifier.

Additionally, the behavior of projectHealth (more precise-computeAdvice) task logic should be adjusted - instead of providing invalid advice it should fail with diagnostics like inconsistent input state or smth like this. As currently it just gives misleading outputs.

Reproducing locally

There are several known scenarios when this happens

  • Module is moved from root project to included build
  • Module is moved from one included build to another included build
  • Same dependant modules are analyzed from two different root isolated builds
    See instructions how to reproduce here: Fix ArtifactsReportTask caching for included builds #1168 (comment)
  • The build script changes the substitution groupId of its modules

Solutions

Several possible solution strategies are suggested.

Rearrange the implementation

In most cases the projectHealth is producing wrong advice, because it (reuses wrong cached output of mentioned tasks and) cannot find matching module by their fully qualified coordinates. Is it possible not to rely on fully qualified coordinates and use internal coordinates instead?
According to #916 we now use IncludedBuildCoordinates intentionally (cc @jjohannes).

Disable caching for artifactsReportX and graphViewX

This solution has obvious disadvantage, but as pros:

  • simple and reliable
  • reduces load on the build cache

This approach can be optional. E.g. if it's known that there are included builds in the project, the caching of task may be disabled.

Related change: #1202 reproducible outputs of tasks to enhance caching of other tasks in build graph

Add new task inputs

Root project name as a task input

Does not cover all cases

Root project full/relative path as a task input

Does not cover all cases

Include variant capabilities as task input as they reflect the difference of isolated builds

PoC: #1168
Does not cover the scenario with moving module from one included build to another, but looks like a nice first step

Use configuration.incoming.resolutionResult.rootComponent as task input

Slows down initialization phase (a bit). Looks most promising and most correct

Related issues:

@autonomousapps autonomousapps added bug Something isn't working performance labels Jul 13, 2024
@autonomousapps autonomousapps added this to the next milestone Jul 13, 2024
@autonomousapps
Copy link
Owner

Thanks for the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance
Projects
None yet
Development

No branches or pull requests

2 participants