The recommended substitute for clearTask of popUpTo with popUpToInclusive only works from start destination actions [116831650]

Fixed

Bug

Status Update

No update yet.

Description

[Deleted User]

created issue #1

Sep 28, 2018 10:32AM

I was previously using clearTask in my navigation actions to make sure that the user could not navigate back to the previous destination (useful for splash screens and login screens). After it was deprecated I decided to try the recommended approach of instead setting popUpTo pointing to the root of the graph together with setting popUpToInclusive="true", but this didn't have the effect I wanted.

This approach only seem to work on actions originating from the graph's start destination, and fails on actions between any other destinations. I created a sample project which reproduces the issue:

https://github.com/hsson/android-navigation-component-alpha06-bug

Here is the project's README detailing the issue:

# Bug in Android Navigation Component (alpha06)
(Note: I have only verified this in alpha06, I don't know if it exists in
previous versions)

## The problem
Basically the problem is that the NavOption "clearTask" was removed in
alpha02, and the recommended substitute was to use app:popUpTo pointing to
the root of the graph/the graph ID, together with app:popUpToInclusive="true".
However, this method does not work. It only seems to work for actions
going from the graph's start destination. If we have the following graph:

x y
A ---> B ---> C

Where A, B, and C are fragments, A is the start destination of the grapg,
and x, y are actions going from A to B and B to C respectively. Let's say
the graph's ID is "@+id/graph"

Then, if both actions x and y have
app:popUpTo="@+id/graph"
app:popUpToInclusive="true"

Then when navigating from A to B through x, and then hitting the back button
will exit the app (as expected). But if navigating from A to B to C through
x and y, and then hitting the back button we will navigate back to B — this
is not what was expected. The expected behavior with this graph is to always
exit the app when clicking the back button

## Environment
This was verified using:
Android Studio 3.2 RC 3
Build #AI-181.5540.7.32.4987877, built on August 31, 2018
JRE: 1.8.0_152-release-1136-b06 x86_64
JVM: OpenJDK 64-Bit Server VM by JetBrains s.r.o
macOS 10.13.6

Comments

[Deleted User] <[Deleted User]> #2Oct 1, 2018 08:14AM

It looks like we have more directories

›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/arm64-v8a/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/arm64-v8a/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/armeabi-v7a/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/armeabi-v7a/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/x86/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/x86/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/x86_64/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/debug/x86_64/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/arm64-v8a/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/arm64-v8a/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/armeabi-v7a/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/armeabi-v7a/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/x86/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/x86/build.ninja.txt📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/x86_64/build.ninja📋
›../../out/androidx/appsearch/appsearch-local-storage/build/../nativeBuildStaging/cmake/release/x86_64/build.ninja.txt📋

it also considers all of these input to configuration cache

il...@google.com <il...@google.com> Oct 1, 2018 10:56PM

Assigned to il...@google.com.

[Deleted User] <[Deleted User]> #3Oct 10, 2018 09:20AM

Prefab feature also seems to cause the following to be added as configuration ache inputs

›../../out/androidx/external/libyuv/build/intermediates/prefab_package_header_only/prefab_publication.json/debug📋
›../../out/androidx/external/libyuv/build/intermediates/prefab_package_header_only/prefab_publication.json/release📋
›../../out/androidx/test/ext/junit-gtest/build/intermediates/prefab_package_header_only/prefab_publication.json/debug📋
›../../out/androidx/test/ext/junit-gtest/build/intermediates/prefab_package_header_only/prefab_publication.json/release📋

il...@google.com <il...@google.com> #4Oct 18, 2018 06:05PM

./gradlew zipTestConfigsWithApks

[Deleted User] <[Deleted User]> #5Oct 19, 2018 11:44AM

./gradlew zipTestConfigsWithApks in the androidx project

Do you mean to run from androidx-main/frameworks/support? If so, I did a run but it took too long and failed in the end with some 9 errors. Even if it finished successfully, it is too long for debug. Do we have to run that specific task or it can be reproduced with some lightweight task? If would be more helpful if we can even reproduce it in a sample project instead of using the androidx project.

Jomo, do you know where those cxx related files are being created? any pointers?

mi...@gmail.com <mi...@gmail.com> #6Oct 19, 2018 12:22PM

The easiest is to probably just look at https://android-build.corp.google.com/builds/pending/P58531861/androidx_incremental/latest/incremental/configuration-cache-reports/78r13m56v7yjbg14vhi2yg2nb/5o4wtl36u55s4qrlzmk4n19px/configuration-cache-report.html

in there see file system entry. all of these files are treated as input to the configuration cache. There should be no entries to ../../out/ once all the plugins are fixed.

[Deleted User] <[Deleted User]> #7Oct 19, 2018 12:24PM

Sadly, the report doesnt give you traces (I asked rodrigo from gradle to add it in the future version of gradle), so we only see it is coming from plugin 'com.android.internal.library'

il...@google.com <il...@google.com> #8Oct 19, 2018 03:36PM

As far as I know, the most effective way to get the traces is to debug a build running with gradle by adding a break point here https://github.com/gradle/gradle/blob/8129d2feae87affd001b82ada7fa57c233f66467/subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java#L367 which is why we need a sample project. Other than that, we have to guess based on path name from the report.

mi...@gmail.com <mi...@gmail.com> #9Oct 19, 2018 03:50PM

The folder path '../../out/androidx/appsearch/appsearch-local-storage/build/intermediates/cxx' originates from ProjectInfo::getIntermediatesDir() which is deprecated with a message that says "Use the version that returns a provider".

The files mentioned in comment#2 are created in ExternalNativeJsonGenerator. The presence of "build/../nativeBuildStaging" in the path indicates that the output location is customized in build.gradle by setting com.android.build.api.dsl.Cmake::buildStagingDirectory.

The files in comment#3 are related to a Gradle module with C++ depending on another Gradle module with C++. Though I don't see why they would have 'debug' or 'release' post-pended. You can exercise the general module-to-module logic by running ModuleToModuleDepsTest.

A question maybe for Ivan, I thought the tests under com.android.build.gradle.integration.ndk.* would fail if they violated configuration caching (I recall the original push for configuration caching required some of these tests to be temporarily exempted). Is there a new constraint arriving with Gradle 8.1? In other words, are these regressions or have they always existed? It might help narrow down the cause/fix.

il...@google.com <il...@google.com> #10Oct 19, 2018 04:13PM

Re #8:

if you upgrade androidx to 8.1 and run the following:

./gradlew appsearch:appsearch-local-storage:assembleDebug

you'll get that code path triggered.

There is also the whole gerrit topic you can apply if you want the 8.2-alpha** agp.

Re #9:

Gradle now instruments all file calls (exists(), isDirectory(), etc) and makes them part of configuration cache input if it happens during the configuration. This is a new requirement starting 8.1

mi...@gmail.com <mi...@gmail.com> #11Oct 19, 2018 04:16PM

Gradle now instruments all file calls (exists(), isDirectory(), etc) and makes them part of configuration cache input if it happens during the configuration. This is a new requirement starting 8.1

In that case, we should be able to find the issues directly by running tests under com.android.build.gradle.integration.ndk.* and failing when configuration caching misses now when it didn't before (I have no idea how to do that though).

dc...@gmail.com <dc...@gmail.com> #12Oct 23, 2018 05:19PM

Note, this affects the second run, so you need to make sure the tests call for a second build and validate that there is a cache hit on the second run.

da...@gmail.com <da...@gmail.com> #13Oct 26, 2018 10:58PM

Similar checks for lint added here

il...@google.com <il...@google.com> #14Nov 30, 2018 11:21PM

Marked as fixed.

Thanks for your input Jomo! Do you have some cycles helping us making cxx compatible with the new constraint of config caching?

Running build twice in a row for cxx integration tests like com.android.build.gradle.integration.ndk.* and check if we have a config cache hit using link sounds like a good way to reproduce the problem.

For example, add the following code at here can reproduce part of the problem

        val result = executor.run(":app:build${appBuildSystem.build}Debug[arm64-v8a]")
        result.assertConfigurationCacheHit()

The way recommended by Gradle is to fix is to do the file calls inside ValueSource. AGP has a "helper" class ConfigPhaseFileCreator which is used for that purpose. An example of how we fix file creation issue during config phase is here