ComponentTreeDepsProcessor generates code in a non-deterministic order, breaking caching

[shashachu]

We're still on v2.38.1 so it's possible this has been fixed, but I didn't notice anything in the release notes.

We don't have solid repro steps, but we've noticed that every so often, our HiltApplication_ComponentTreeDeps.java file generates a file that has the same content, but different ordering, causing an unnecessary rebuild.

It seems to happen within the aggregatedDeps property of the @ComponentTreeDeps annotation, although I don't know that it's limited to that.

[bcorso]

Hi @shashachu,

Just to help narrow this down, are you using hilt { enableAggregatingTask = true }?

[shashachu]

@bcorso we are not.

[shashachu]

@bcorso is this something that's possibly improved by the enableAggregatingTask flag? if so, we could experiment with that.

[bcorso]

The main reason I asked is because there's two different code paths used to generate @ComponentTreeDeps for when the flag is enabled vs disabled. Thus, it would help to narrow down where to look if we knew if the issue happened with one, the other, or both.

In general, I would recommend using enableAggregatingTask = true regardless (it's on by default in Dagger 2.40+) because it makes your builds more performant and safer.

If decide to start using enableAggregatingTask and the issue still remains that information would also help.

[bcorso]

I think I found the culprit (or at least one of them, hard to say since we don't have a good way to repro).

I'll send out a fix in a bit.

[shashachu]

perfect, thanks so much for the quick turnaround. Is this only relevant in the enableAggregatingTask = false flow? I assume you'll be merging the change into 2.4.0, so we'd need to upgrade either way.

[bcorso]

It's in code shared by both (https://github.com/google/dagger/pull/3009/files), so it will be relevant whether you enable the flag or not.

Yeah, it will require an update (should be in v2.40.1 which will likely be released before the end of the week).

[bcorso]

Thanks for pointing out that mutableSetOf returns LinkedHashSet by default... so back to the drawing board!

I took a look through the remaining code and didn't find anything else that stood out.

Are you using Gradle? If you have a small sample project that reproduces this (even if it's infrequent) that would be great. I want to try to repro this and do some debugging locally.

Worst case scenario, we can try just manually sorting before generating the annotation, but I'd like to find the root cause if possible.

[shashachu]

Are you using Gradle? If you have a small sample project that reproduces this (even if it's infrequent) that would be great. I want to try to repro this and do some debugging locally.

Yes, we are. We haven't tried making a sample project, but I can give it a shot. In our app (Pinterest) it seems to happen pretty randomly. For now we've started archiving a few Hilt-generated files so that we can at least compare the contents when it does look like it causes a cache miss.

Worst case scenario, we can try just manually sorting before generating the annotation, but I'd like to find the root cause if possible.

This is what we ended up doing with our own annotation processors (added a RoundEnvironment.getSortedElementsAnnotatedWith extension function).

[shashachu]

@bcorso this project my colleague created for a different bug report is reproducing the issue consistently for us.

• Clone https://github.com/techeretic/HiltRobolectricIssue
• Run ./gradlew clean assembleDebug
• Make a trivial non-Hilt-related change. I added an empty override to MainActivity:

override fun onCreate(savedInstanceState: Bundle?, persistentState: PersistableBundle?) {
    super.onCreate(savedInstanceState, persistentState)
}

• Run ./gradlew assembleDebug
  The contents of aggregatedDeps in HiltMainApplication_ComponentTreeDeps.java changes ordering:

79c79
<         _com_bug_hiltrobolectricissue_di_modules_FirstModule.class,
---
>         _com_bug_hiltrobolectricissue_MainApplication_Dependencies.class,
81c81
<         _com_bug_hiltrobolectricissue_HiltMainApplication_GeneratedInjector.class,
---
>         _com_bug_hiltrobolectricissue_di_modules_FirstModule.class,
83,84c83,84
<         _com_bug_hiltrobolectricissue_MainApplication_Dependencies.class,
<         _com_bug_hiltrobolectricissue_MainActivity_GeneratedInjector.class
---
>         _com_bug_hiltrobolectricissue_MainActivity_GeneratedInjector.class,
>         _com_bug_hiltrobolectricissue_HiltMainApplication_GeneratedInjector.class

I noticed that when turning on enableAggregatingTask, the ComponentDeps file isn't generated, so perhaps that flag avoids the issue that way, but I can't say for sure there isn't some other issue when that flag is on.

Let me know if you have any trouble reproducing the issue.

[shashachu]

Also - I upgraded to 2.40.1 in the sample project (with enableAggregatingTask = false) and the issue persists (which is expected.)

[bcorso]

@shashachu hmm, I couldn't repro this using the linked project. I tried rebuilding around 20 times (even cleaning in between) but the order remained consistent.

Is there anything else that could be different? Which version of Java are you building with?

[shashachu]

@bcorso oh how strange. I had a coworker verify he could also reproduce before commenting here. We're using Java 11, the version bundled with Android Studio Arctic Fox. I'm on PTO this week but I'll ask a colleague to comment with the exact version.

[shashachu]

Also - just want to double check that you saw the step where you need to make a trivial change to cause a rebuild.

[techeretic]

Hi @bcorso , I'm @shashachu 's colleague. We're using the AS embedded JDK, i.e. 11.0.10 for building that project.

[bcorso]

Ah, thanks I did miss the part about making the change. I am now able to reproduce it, thanks!

[bcorso]

Playing around with it a bit, I think the order is correlated to what is being built. In particular, a clean build will always reproduce the same output, but an incremental build will produce different output depending on which sources needed to be rebuilt. IIUC, when we're grabbing elements from a package, the most recently processed sources will appear last in the list.

Thus, we can't hope for a well defined ordering of elements from these packages, and I think our only option here is just to manually sort them ourselves.

Thanks for reporting! I'll look into getting a fix for this soon.

[shashachu]

That's interesting. Do you know if the processing order is driven by kapt? Gradle? It's a pretty subtle gotcha about annotation processors, especially because it can seem random. Just wondering if there's some larger bug that should be reported, or if the order of elements is not guaranteed, by design.

[bcorso]

Do you know if the processing order is driven by kapt? Gradle?

I'm not 100% certain, but my observation is that for a given input to the build, the output is well defined. Thus, I'd guess that it's the incremental processing of Gradle that is changing the inputs to the build and thus changing the output. So in that sense, I guess you could say that Gradle is driving the ordering. However, I'm not familiar enough with Gradle's incremental processing to know what the guarantees are in this case.

[tinder-inakivillar]

Hi, I'm able to reproduce this issue with totally clean builds populating the cache from CI and consuming the outputs from clean local builds.
The error only happens in the main entry point module, the libraries are ok

[bcorso]

@tinder-inakivillar do you mean that on consecutive calls to ./gradlew clean assembleDebug you get different output?

[tinder-inakivillar]

I'm testing cache ratio for local clean builds, no incremental builds. I populate the Cache with a clean build in CI(linux). Then removing my build-cache-1 locally I execute for the same commit the main task for the project(Mac).
I'm able to compare both builds with Build Scans(generating task inputs) and is there where I saw the difference on the file: DebugXXXApplication_ComponentTreeDeps.java

I compared manually the files and I observed the content is the same but the order at the end if the file is different for around 40 lines.

[shashachu]

Woohoo! Thanks for the quick fix.