From 5f364b63b89bebc75ceb3d04b9efc4fe8aae26cd Mon Sep 17 00:00:00 2001 From: Ulya Trafimovich Date: Tue, 30 Jun 2020 12:39:01 +0100 Subject: [PATCH] Rewrite construct_context.sh in Python. This allows to reuse SDK version comparison routine from other Python scripts. With the addition of non-numeric versions comparison has become more complex, and the deleted shell script did it incorrectly: it used comparisons like `[[ "S" -lt 28 ]]` which results in "true" as strings are converted to zero in numeric context. This resulted in adding legacy libraries to class loader context of apps targeting recent SDK versions. The error was masked because currently there is only one non-AOSP app that uses the script (GoogleDialer), and it targets numeric SDK version. Test: lunch aosp_cf_x86_phone-userdebug && m Bug: 132357300 Change-Id: I26e752e29b5453fd3e29d5dbfbe4d9df9c0a55b7 --- dexpreopt/config.go | 4 +- dexpreopt/dexpreopt.go | 23 ++++---- java/app.go | 5 +- java/app_test.go | 4 +- scripts/Android.bp | 40 ++++++++++++++ scripts/OWNERS | 2 +- scripts/construct_context.py | 90 +++++++++++++++++++++++++++++++ scripts/construct_context.sh | 78 --------------------------- scripts/construct_context_test.py | 81 ++++++++++++++++++++++++++++ 9 files changed, 229 insertions(+), 98 deletions(-) create mode 100755 scripts/construct_context.py delete mode 100755 scripts/construct_context.sh create mode 100755 scripts/construct_context_test.py diff --git a/dexpreopt/config.go b/dexpreopt/config.go index 4a4e834be..2cf65feca 100644 --- a/dexpreopt/config.go +++ b/dexpreopt/config.go @@ -383,7 +383,7 @@ func createGlobalSoongConfig(ctx android.ModuleContext) *GlobalSoongConfig { SoongZip: ctx.Config().HostToolPath(ctx, "soong_zip"), Zip2zip: ctx.Config().HostToolPath(ctx, "zip2zip"), ManifestCheck: ctx.Config().HostToolPath(ctx, "manifest_check"), - ConstructContext: android.PathForSource(ctx, "build/soong/scripts/construct_context.sh"), + ConstructContext: ctx.Config().HostToolPath(ctx, "construct_context"), } } @@ -574,7 +574,7 @@ func GlobalSoongConfigForTests(config android.Config) *GlobalSoongConfig { SoongZip: android.PathForTesting("soong_zip"), Zip2zip: android.PathForTesting("zip2zip"), ManifestCheck: android.PathForTesting("manifest_check"), - ConstructContext: android.PathForTesting("construct_context.sh"), + ConstructContext: android.PathForTesting("construct_context"), } }).(*GlobalSoongConfig) } diff --git a/dexpreopt/dexpreopt.go b/dexpreopt/dexpreopt.go index edfd2ad0f..e49fa984e 100644 --- a/dexpreopt/dexpreopt.go +++ b/dexpreopt/dexpreopt.go @@ -208,7 +208,7 @@ type classLoaderContext struct { // targetSdkVersion in the manifest or APK is less than that API version. type classLoaderContextMap map[int]*classLoaderContext -const anySdkVersion int = -1 +const anySdkVersion int = 9999 // should go last in class loader context func (m classLoaderContextMap) getSortedKeys() []int { keys := make([]int, 0, len(m)) @@ -340,22 +340,21 @@ func dexpreoptCommand(ctx android.PathContext, globalSoong *GlobalSoongConfig, g Text(`)"`) } - // Generate commands that define shell variables for versioned classpaths - // and construct class loader context from them using construct_context.sh. + // Generate command that saves host and target class loader context in shell variables. + cmd := rule.Command(). + Text(`eval "$(`).Tool(globalSoong.ConstructContext). + Text(` --target-sdk-version ${target_sdk_version}`) for _, ver := range classLoaderContexts.getSortedKeys() { clc := classLoaderContexts.getValue(ver) - var varHost, varTarget string + verString := fmt.Sprintf("%d", ver) if ver == anySdkVersion { - varHost = "dex_preopt_host_libraries" - varTarget = "dex_preopt_target_libraries" - } else { - varHost = fmt.Sprintf("conditional_host_libs_%d", ver) - varTarget = fmt.Sprintf("conditional_target_libs_%d", ver) + verString = "any" // a special keyword that means any SDK version } - rule.Command().Textf(varHost+`="%s"`, strings.Join(clc.Host.Strings(), " ")).Implicits(clc.Host) - rule.Command().Textf(varTarget+`="%s"`, strings.Join(clc.Target, " ")) + cmd.Textf(`--host-classpath-for-sdk %s %s`, verString, strings.Join(clc.Host.Strings(), ":")). + Implicits(clc.Host). + Textf(`--target-classpath-for-sdk %s %s`, verString, strings.Join(clc.Target, ":")) } - rule.Command().Text("source").Tool(globalSoong.ConstructContext).Input(module.DexPath) + cmd.Text(`)"`) } else { // Pass special class loader context to skip the classpath and collision check. // This will get removed once LOCAL_USES_LIBRARIES is enforced. diff --git a/java/app.go b/java/app.go index 37a6453cd..6a806a914 100755 --- a/java/app.go +++ b/java/app.go @@ -1860,9 +1860,8 @@ func (u *usesLibrary) deps(ctx android.BottomUpMutatorContext, hasFrameworkLibs // creating a cyclic dependency: // e.g. framework-res -> org.apache.http.legacy -> ... -> framework-res. if hasFrameworkLibs { - // dexpreopt/dexpreopt.go needs the paths to the dex jars of these libraries in case construct_context.sh needs - // to pass them to dex2oat. Add them as a dependency so we can determine the path to the dex jar of each - // library to dexpreopt. + // Dexpreopt needs paths to the dex jars of these libraries in order to construct + // class loader context for dex2oat. Add them as a dependency with a special tag. ctx.AddVariationDependencies(nil, usesLibTag, "org.apache.http.legacy", "android.hidl.base-V1.0-java", diff --git a/java/app_test.go b/java/app_test.go index 120dc00a6..e0c5820ea 100644 --- a/java/app_test.go +++ b/java/app_test.go @@ -2587,13 +2587,13 @@ func TestUsesLibraries(t *testing.T) { // Test that only present libraries are preopted cmd = app.Rule("dexpreopt").RuleParams.Command - if w := `dex_preopt_target_libraries="/system/framework/foo.jar /system/framework/bar.jar"`; !strings.Contains(cmd, w) { + if w := `--target-classpath-for-sdk any /system/framework/foo.jar:/system/framework/bar.jar`; !strings.Contains(cmd, w) { t.Errorf("wanted %q in %q", w, cmd) } cmd = prebuilt.Rule("dexpreopt").RuleParams.Command - if w := `dex_preopt_target_libraries="/system/framework/foo.jar /system/framework/bar.jar"`; !strings.Contains(cmd, w) { + if w := `--target-classpath-for-sdk any /system/framework/foo.jar:/system/framework/bar.jar`; !strings.Contains(cmd, w) { t.Errorf("wanted %q in %q", w, cmd) } } diff --git a/scripts/Android.bp b/scripts/Android.bp index 1f5503051..ac2f42fb0 100644 --- a/scripts/Android.bp +++ b/scripts/Android.bp @@ -149,6 +149,46 @@ python_test_host { test_suites: ["general-tests"], } +python_binary_host { + name: "construct_context", + main: "construct_context.py", + srcs: [ + "construct_context.py", + ], + version: { + py2: { + enabled: true, + }, + py3: { + enabled: false, + }, + }, + libs: [ + "manifest_utils", + ], +} + +python_test_host { + name: "construct_context_test", + main: "construct_context_test.py", + srcs: [ + "construct_context_test.py", + "construct_context.py", + ], + version: { + py2: { + enabled: true, + }, + py3: { + enabled: false, + }, + }, + libs: [ + "manifest_utils", + ], + test_suites: ["general-tests"], +} + python_binary_host { name: "lint-project-xml", main: "lint-project-xml.py", diff --git a/scripts/OWNERS b/scripts/OWNERS index 027455484..8c644247e 100644 --- a/scripts/OWNERS +++ b/scripts/OWNERS @@ -1,4 +1,4 @@ per-file system-clang-format,system-clang-format-2 = enh@google.com,smoreland@google.com per-file build-mainline-modules.sh = ngeoffray@google.com,paulduffin@google.com,mast@google.com per-file build-aml-prebuilts.sh = ngeoffray@google.com,paulduffin@google.com,mast@google.com -per-file construct_context.sh = ngeoffray@google.com,calin@google.com,mathieuc@google.com,skvadrik@google.com +per-file construct_context.py = ngeoffray@google.com,calin@google.com,mathieuc@google.com,skvadrik@google.com diff --git a/scripts/construct_context.py b/scripts/construct_context.py new file mode 100755 index 000000000..8717fe305 --- /dev/null +++ b/scripts/construct_context.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python +# +# Copyright (C) 2020 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""A tool for constructing class loader context.""" + +from __future__ import print_function + +import argparse +import sys + +from manifest import compare_version_gt + + +def parse_args(args): + """Parse commandline arguments.""" + parser = argparse.ArgumentParser() + parser.add_argument('--target-sdk-version', default='', dest='sdk', + help='specify target SDK version (as it appears in the manifest)') + parser.add_argument('--host-classpath-for-sdk', dest='host_classpaths', + action='append', nargs=2, metavar=('sdk','classpath'), + help='specify classpath on host for a given SDK version or "any" version') + parser.add_argument('--target-classpath-for-sdk', dest='target_classpaths', + action='append', nargs=2, metavar=('sdk','classpath'), + help='specify classpath on target for a given SDK version or "any" version') + return parser.parse_args(args) + +# The hidl.manager shared library has a dependency on hidl.base. We manually +# add that information to the class loader context if we see those libraries. +HIDL_MANAGER = 'android.hidl.manager-V1.0-java' +HIDL_BASE = 'android.hidl.base-V1.0-java' + +# Special keyword that means that the classpath should be added to class loader +# context regardless of the target SDK version. +any_sdk = 'any' + +# We assume that the order of classpath arguments passed to this script is +# correct (matches the order computed by package manager). It is possible to +# sort them here, but Soong needs to use deterministic order anyway, so it can +# as well use the correct order. +def construct_context(versioned_classpaths, target_sdk): + context = [] + for [sdk, classpath] in versioned_classpaths: + if sdk == any_sdk or compare_version_gt(sdk, target_sdk): + for jar in classpath.split(':'): + pcl = 'PCL[%s]' % jar + if HIDL_MANAGER in jar: + pcl += '{PCL[%s]}' % jar.replace(HIDL_MANAGER, HIDL_BASE, 1) + context.append(pcl) + return context + +def construct_contexts(args): + host_context = construct_context(args.host_classpaths, args.sdk) + target_context = construct_context(args.target_classpaths, args.sdk) + context_sep = '#' + return ('class_loader_context_arg=--class-loader-context=PCL[]{%s} ; ' % context_sep.join(host_context) + + 'stored_class_loader_context_arg=--stored-class-loader-context=PCL[]{%s}' % context_sep.join(target_context)) + +def main(): + """Program entry point.""" + try: + args = parse_args(sys.argv[1:]) + if not args.sdk: + raise SystemExit('target sdk version is not set') + if not args.host_classpaths: + raise SystemExit('host classpath is not set') + if not args.target_classpaths: + raise SystemExit('target classpath is not set') + + print(construct_contexts(args)) + + # pylint: disable=broad-except + except Exception as err: + print('error: ' + str(err), file=sys.stderr) + sys.exit(-1) + +if __name__ == '__main__': + main() diff --git a/scripts/construct_context.sh b/scripts/construct_context.sh deleted file mode 100755 index d620d0846..000000000 --- a/scripts/construct_context.sh +++ /dev/null @@ -1,78 +0,0 @@ -#!/bin/bash -# -# Copyright (C) 2018 The Android Open Source Project -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -set -e - -# target_sdk_version: parsed from manifest -# -# outputs -# class_loader_context_arg: final class loader conext arg -# stored_class_loader_context_arg: final stored class loader context arg - -if [ -z "${target_sdk_version}" ]; then - echo "ERROR: target_sdk_version not set" - exit 2 -fi - -# The hidl.manager shared library has a dependency on hidl.base. We'll manually -# add that information to the class loader context if we see those libraries. -hidl_manager="android.hidl.manager-V1.0-java" -hidl_base="android.hidl.base-V1.0-java" - -function add_to_contexts { - for i in $1; do - if [[ -z "${class_loader_context}" ]]; then - export class_loader_context="PCL[$i]" - else - export class_loader_context+="#PCL[$i]" - fi - if [[ $i == *"$hidl_manager"* ]]; then - export class_loader_context+="{PCL[${i/$hidl_manager/$hidl_base}]}" - fi - done - - for i in $2; do - if [[ -z "${stored_class_loader_context}" ]]; then - export stored_class_loader_context="PCL[$i]" - else - export stored_class_loader_context+="#PCL[$i]" - fi - if [[ $i == *"$hidl_manager"* ]]; then - export stored_class_loader_context+="{PCL[${i/$hidl_manager/$hidl_base}]}" - fi - done -} - -# The order below must match what the package manager also computes for -# class loader context. - -if [[ "${target_sdk_version}" -lt "28" ]]; then - add_to_contexts "${conditional_host_libs_28}" "${conditional_target_libs_28}" -fi - -if [[ "${target_sdk_version}" -lt "29" ]]; then - add_to_contexts "${conditional_host_libs_29}" "${conditional_target_libs_29}" -fi - -if [[ "${target_sdk_version}" -lt "30" ]]; then - add_to_contexts "${conditional_host_libs_30}" "${conditional_target_libs_30}" -fi - -add_to_contexts "${dex_preopt_host_libraries}" "${dex_preopt_target_libraries}" - -# Generate the actual context string. -export class_loader_context_arg="--class-loader-context=PCL[]{${class_loader_context}}" -export stored_class_loader_context_arg="--stored-class-loader-context=PCL[]{${stored_class_loader_context}}" diff --git a/scripts/construct_context_test.py b/scripts/construct_context_test.py new file mode 100755 index 000000000..0b0b0a315 --- /dev/null +++ b/scripts/construct_context_test.py @@ -0,0 +1,81 @@ +#!/usr/bin/env python +# +# Copyright (C) 2020 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""Unit tests for construct_context.py.""" + +import sys +import unittest + +import construct_context as cc + +sys.dont_write_bytecode = True + +def construct_contexts(arglist): + args = cc.parse_args(arglist) + return cc.construct_contexts(args) + +classpaths = [ + '--host-classpath-for-sdk', '28', 'out/zdir/z.jar', + '--target-classpath-for-sdk', '28', '/system/z.jar', + '--host-classpath-for-sdk', '29', 'out/xdir/x.jar:out/ydir/y.jar', + '--target-classpath-for-sdk', '29', '/system/x.jar:/product/y.jar', + '--host-classpath-for-sdk', 'any', 'out/adir/a.jar:out/android.hidl.manager-V1.0-java.jar:out/bdir/b.jar', + '--target-classpath-for-sdk', 'any', '/system/a.jar:/system/android.hidl.manager-V1.0-java.jar:/product/b.jar', +] + +class ConstructContextTest(unittest.TestCase): + def test_construct_context_28(self): + args = ['--target-sdk-version', '28'] + classpaths + result = construct_contexts(args) + expect = ('class_loader_context_arg=--class-loader-context=PCL[]{PCL[out/xdir/x.jar]' + '#PCL[out/ydir/y.jar]' + '#PCL[out/adir/a.jar]' + '#PCL[out/android.hidl.manager-V1.0-java.jar]{PCL[out/android.hidl.base-V1.0-java.jar]}' + '#PCL[out/bdir/b.jar]}' + ' ; ' + 'stored_class_loader_context_arg=--stored-class-loader-context=PCL[]{PCL[/system/x.jar]' + '#PCL[/product/y.jar]' + '#PCL[/system/a.jar]' + '#PCL[/system/android.hidl.manager-V1.0-java.jar]{PCL[/system/android.hidl.base-V1.0-java.jar]}' + '#PCL[/product/b.jar]}') + self.assertEqual(result, expect) + + def test_construct_context_29(self): + args = ['--target-sdk-version', '29'] + classpaths + result = construct_contexts(args) + expect = ('class_loader_context_arg=--class-loader-context=PCL[]{PCL[out/adir/a.jar]' + '#PCL[out/android.hidl.manager-V1.0-java.jar]{PCL[out/android.hidl.base-V1.0-java.jar]}' + '#PCL[out/bdir/b.jar]}' + ' ; ' + 'stored_class_loader_context_arg=--stored-class-loader-context=PCL[]{PCL[/system/a.jar]' + '#PCL[/system/android.hidl.manager-V1.0-java.jar]{PCL[/system/android.hidl.base-V1.0-java.jar]}' + '#PCL[/product/b.jar]}') + self.assertEqual(result, expect) + + def test_construct_context_S(self): + args = ['--target-sdk-version', 'S'] + classpaths + result = construct_contexts(args) + expect = ('class_loader_context_arg=--class-loader-context=PCL[]{PCL[out/adir/a.jar]' + '#PCL[out/android.hidl.manager-V1.0-java.jar]{PCL[out/android.hidl.base-V1.0-java.jar]}' + '#PCL[out/bdir/b.jar]}' + ' ; ' + 'stored_class_loader_context_arg=--stored-class-loader-context=PCL[]{PCL[/system/a.jar]' + '#PCL[/system/android.hidl.manager-V1.0-java.jar]{PCL[/system/android.hidl.base-V1.0-java.jar]}' + '#PCL[/product/b.jar]}') + self.assertEqual(result, expect) + +if __name__ == '__main__': + unittest.main(verbosity=2)