Resource API tracking should treat the "no <public> elements" case as all resources being public, rather than no resources being public [170882230]

Fixed

Bug

Status Update

No update yet.

Description

al...@google.com

created issue #1

Oct 14, 2020 07:49PM

Our resource API tracking inverts the behavior of AGP by treating the "no public element" case as "all resources are private" when in fact it indicates the legacy "all resources are public" behavior.

The correct way to declare "no public elements" is to define a single <public /> element.

Unclear how we're going to handle this for existing libraries where all resources are currently considered public but have never been tracked.

Comments

al...@google.com <al...@google.com> #2Nov 2, 2020 09:15PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

al...@google.com <al...@google.com> #3Nov 2, 2020 10:11PM

Going to move forward with "fixing" the libraries which accidentally exposed all their resources as public.

Tentatively, we'll:

Require a public.txt to exist in the AAR, with a failure message along the lines of "At least one public resource must be defined. If your library has no public resources then please define a single, empty <public /> element in res/values/public.xml."
Add a public.xml file as described to all failing libraries
Be very, very explicit about the change in release notes
Deal with the fallout in g3

Estimate is 2 due to g3 issues.

al...@google.com <al...@google.com> Jan 8, 2021 09:33PM

Accepted by al...@google.com.

al...@google.com <al...@google.com> #4Jan 8, 2021 11:06PM

(1) in comment#3 might not be feasible. It seems like "are there resources defined that will be public" might only be available within AAPT. We could check whether any resources are defined in the source set and then see whether there's a public.txt output from MergeResources task, but I'm not confident that would map to "all resources are public by default".

+cc'ing Izabela who might know how to get at this information from within a Gradle plugin.

We could also skip (1) and implement (2) by forcing all libraries to include a resource XML with a single <public /> element.

Message last modified on Jan 8, 2021 11:09PM

im...@google.com <im...@google.com> #5Jan 11, 2021 12:22PM

Currently, in the non-namespaced builds (defaults), AAPT is completely unaware of resource visibility, since we filter all the <public> tags during resource merging (to create public.txt).

I agree that "no public tag means everything is public" is a nuisance, and gave us a lot of hassle when implementing the conversion of AARs to namespaced ones (which as you know are very strict with res visibility). While we can't change that behaviour for backwards compatibility, we could propose changing to always generating public.txt (filled with *all* local resources if no public tags were defined) - It would increase the AAR size, but it's probably negligible.

For now, you should be able to consume the public.txt from MergeResources (the "PACKAGE" one, since in libraries you will have two MergeResources tasks, the other one being "MERGE"), by extra gradle Task that sees if the file exists - if it doesn't then fail and say "you don't have a <public> tag defined, if you need all resources to be public you need to list them all" or whatever matches your case.

I think Chris has done a lot of work around the APIs recently, and added a public API for getting the public.txt in GenerateApiPublicTxtTask and the artifact ArtifactType.PUBLIC_ANDROID_RESOURCES_LIST. However, this one's behaviour is different, meaning that if there were no <public> tags, it will generate the public.txt will *all* symbols (instead of not writing it). The one matching the AAR's public.txt would be InternalArtifactType.PUBLIC_RES but that one is our internal artifact.

al...@google.com <al...@google.com> #6Jan 11, 2021 03:34PM

if it doesn't then fail and say "you don't have a <public> tag defined, if you need all resources to be public you need to list them all" or whatever matches your case.

I was hoping to avoid this for libraries that don't have any resources defined.

However, this one's behaviour is different, meaning that if there were no <public> tags, it will generate the public.txt will all symbols (instead of not writing it).

I could correlate between the two -- if the AAR does not have a public.txt but GenerateApiPublicTxtTask has a non-empty public.txt then the developer has added resources but no explicit <public />.

Which feels hacky. I think we'll just force every library to have a single <public /> element by default. One less thing for developers to think about.

al...@google.com <al...@google.com> #7Jan 11, 2021 08:39PM

Forcing every library to have a <public /> element ended up being a bit hacky as well since some libraries were already correct and the build will fail if you define <public /> twice. I ended up removing all the existing <public /> definitions in aosp/1546636.

ap...@google.com <ap...@google.com> #8Jan 14, 2021 06:50PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 721ee0257e22d51a901eab77d8ba07112b608d55
Author: Alan Viverette <alanv@google.com>
Date: Mon Jan 11 10:55:42 2021

Always define public resources

Relnote: "Resources in libraries with no explicily declared public resources
(ex. via public.xml) are now private by default."
Fixes: 170882230
Test: ./gradlew checkResourceApi
Change-Id: Ia1dcca1ad5c65c1ab90b97c22589e7392161fd62

A buildSrc/res/values/public.xml
M buildSrc/src/main/kotlin/androidx/build/AndroidXPlugin.kt
M buildSrc/src/main/kotlin/androidx/build/resources/GenerateResourceApiTask.kt
A buildSrc/src/main/kotlin/androidx/build/resources/PublicResourcesStubHelper.kt
M camera/camera-camera2/src/main/res/values/public.xml
M camera/camera-core/src/main/res/values/public.xml
M compose/ui/ui/src/androidMain/res/values/public.xml
M fragment/fragment/src/main/res/values/public.xml

https://android-review.googlesource.com/1546636