From 121e284b460afc3603b88883563ad3156aa2de4d Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 14 Sep 2016 21:38:29 -0700 Subject: [PATCH] Fix link_type checking This was printing "error:", but not actually triggering an error. Instead of trying to write a single line bash script to handle this, move the actual check into python. This allows us to print all of the errors for a single module before triggering the failure. Also updates the warning format and the warn.py script to properly parse these warning. Many of the java:sdk -> java:platform warnings are false positives due to the lack of LOCAL_SDK_VERSION markings on prebuilts. Individual tags can be marked as warnings now, which lets us check for system libraries linking against vendor libraries (which won't work on AOSP). I'm not sure this is a completely valid check, which one reason that it's just a warning. Test: m all_link_types (with some missing libs commented out) Change-Id: I333e418c9a4511b7c7e826891ae481da08fbf6f9 --- core/binary.mk | 23 ++++----- core/config.mk | 1 + core/definitions.mk | 29 +++++++++++ core/install_jni_libs_internal.mk | 22 ++++----- core/java_common.mk | 27 +++++------ core/main.mk | 3 ++ core/prebuilt_internal.mk | 14 +++--- tools/check_link_type.py | 80 +++++++++++++++++++++++++++++++ tools/warn.py | 11 ++++- 9 files changed, 164 insertions(+), 46 deletions(-) create mode 100755 tools/check_link_type.py diff --git a/core/binary.mk b/core/binary.mk index b9cff6555a..b521dce844 100644 --- a/core/binary.mk +++ b/core/binary.mk @@ -1365,13 +1365,17 @@ endif #################################################### my_link_type := $(intermediates)/link_type +all_link_types: $(my_link_type) ifdef LOCAL_SDK_VERSION -$(my_link_type): PRIVATE_LINK_TYPE := ndk -$(my_link_type): PRIVATE_ALLOWED_TYPES := ndk +$(my_link_type): PRIVATE_LINK_TYPE := native:ndk +$(my_link_type): PRIVATE_WARN_TYPES := +$(my_link_type): PRIVATE_ALLOWED_TYPES := native:ndk else -$(my_link_type): PRIVATE_LINK_TYPE := platform -$(my_link_type): PRIVATE_ALLOWED_TYPES := (ndk|platform) +$(my_link_type): PRIVATE_LINK_TYPE := native:platform +$(my_link_type): PRIVATE_WARN_TYPES := +$(my_link_type): PRIVATE_ALLOWED_TYPES := native:ndk native:platform endif +$(eval $(call link-type-partitions,$(my_link_type))) my_link_type_deps := $(strip \ $(foreach l,$(my_whole_static_libraries) $(my_static_libraries), \ $(call intermediates-dir-for,STATIC_LIBRARIES,$(l),$(my_kind),,$(LOCAL_2ND_ARCH_VAR_PREFIX),$(my_host_cross))/link_type)) @@ -1383,16 +1387,9 @@ endif $(my_link_type): PRIVATE_DEPS := $(my_link_type_deps) $(my_link_type): PRIVATE_MODULE := $(LOCAL_MODULE) $(my_link_type): PRIVATE_MAKEFILE := $(LOCAL_MODULE_MAKEFILE) -$(my_link_type): $(my_link_type_deps) +$(my_link_type): $(my_link_type_deps) $(CHECK_LINK_TYPE) @echo Check module type: $@ - $(hide) mkdir -p $(dir $@) && rm -f $@ -ifdef my_link_type_deps - $(hide) for f in $(PRIVATE_DEPS); do \ - grep -qE '^$(PRIVATE_ALLOWED_TYPES)$$' $$f || \ - ($(call echo-error,"$(PRIVATE_MAKEFILE): $(PRIVATE_MODULE) ($(PRIVATE_LINK_TYPE)) should not link to $$(basename $${f%_intermediates/link_type}) ($$(cat $$f))"); exit 1) \ - done -endif - $(hide) echo $(PRIVATE_LINK_TYPE) >$@ + $(check-link-type) ########################################################### diff --git a/core/config.mk b/core/config.mk index 6178404b2d..3c2384c180 100644 --- a/core/config.mk +++ b/core/config.mk @@ -586,6 +586,7 @@ TUNE2FS := $(HOST_OUT_EXECUTABLES)/tune2fs$(HOST_EXECUTABLE_SUFFIX) JARJAR := $(HOST_OUT_JAVA_LIBRARIES)/jarjar.jar DATA_BINDING_COMPILER := $(HOST_OUT_JAVA_LIBRARIES)/databinding-compiler.jar FAT16COPY := build/tools/fat16copy.py +CHECK_LINK_TYPE := build/tools/check_link_type.py ifneq ($(ANDROID_JACK_EXTRA_ARGS),) JACK_DEFAULT_ARGS := diff --git a/core/definitions.mk b/core/definitions.mk index 0c169e4f73..29b6539d67 100644 --- a/core/definitions.mk +++ b/core/definitions.mk @@ -3115,6 +3115,35 @@ $(strip $(if $(LOCAL_RECORDED_MODULE_TYPE),, $(error Invalid module type: $(1)))))) endef +########################################################### +# Link type checking +########################################################### +define check-link-type +$(hide) mkdir -p $(dir $@) && rm -f $@ +$(hide) $(CHECK_LINK_TYPE) --makefile $(PRIVATE_MAKEFILE) --module $(PRIVATE_MODULE) \ + --type "$(PRIVATE_LINK_TYPE)" $(addprefix --allowed ,$(PRIVATE_ALLOWED_TYPES)) \ + $(addprefix --warn ,$(PRIVATE_WARN_TYPES)) $(PRIVATE_DEPS) +$(hide) echo "$(PRIVATE_LINK_TYPE)" >$@ +endef + +define link-type-partitions +ifndef LOCAL_IS_HOST_MODULE +ifeq (true,$(LOCAL_PROPRIETARY_MODULE)) +$(1): PRIVATE_LINK_TYPE += partition:vendor +$(1): PRIVATE_ALLOWED_TYPES += partition:vendor partition:oem partition:odm +else ifeq (true,$(LOCAL_OEM_MODULE)) +$(1): PRIVATE_LINK_TYPE += partition:oem +$(1): PRIVATE_ALLOWED_TYPES += partition:vendor partition:oem partition:odm +else ifeq (true,$(LOCAL_ODM_MODULE)) +$(1): PRIVATE_LINK_TYPE += partition:odm +$(1): PRIVATE_ALLOWED_TYPES += partition:vendor partition:oem partition:odm +else +# TODO: Mark libraries in /data +$(1): PRIVATE_WARN_TYPES += partition:vendor partition:oem partition:odm +endif +endif +endef + ########################################################### ## Other includes ########################################################### diff --git a/core/install_jni_libs_internal.mk b/core/install_jni_libs_internal.mk index 9f5d445e0a..14cc384ecd 100644 --- a/core/install_jni_libs_internal.mk +++ b/core/install_jni_libs_internal.mk @@ -102,28 +102,26 @@ endif # outer my_prebuilt_jni_libs # Verify that all included libraries are built against the NDK ifneq ($(strip $(LOCAL_JNI_SHARED_LIBRARIES)),) my_link_type := $(call intermediates-dir-for,APPS,$(LOCAL_MODULE))/$(my_2nd_arch_prefix)jni_link_type +all_link_types: $(my_link_type) my_link_type_deps := $(strip \ $(foreach l,$(LOCAL_JNI_SHARED_LIBRARIES),\ $(call intermediates-dir-for,SHARED_LIBRARIES,$(l),,,$(my_2nd_arch_prefix))/link_type)) ifneq ($(LOCAL_SDK_VERSION),) -$(my_link_type): PRIVATE_LINK_TYPE := sdk -$(my_link_type): PRIVATE_ALLOWED_TYPES := ndk +$(my_link_type): PRIVATE_LINK_TYPE := app:sdk +$(my_link_type): PRIVATE_WARN_TYPES := native:platform +$(my_link_type): PRIVATE_ALLOWED_TYPES := native:ndk else -$(my_link_type): PRIVATE_LINK_TYPE := platform -$(my_link_type): PRIVATE_ALLOWED_TYPES := (ndk|platform) +$(my_link_type): PRIVATE_LINK_TYPE := app:platform +$(my_link_type): PRIVATE_WARN_TYPES := +$(my_link_type): PRIVATE_ALLOWED_TYPES := native:ndk native:platform endif +$(eval $(call link-type-partitions,$(my_link_type))) $(my_link_type): PRIVATE_DEPS := $(my_link_type_deps) $(my_link_type): PRIVATE_MODULE := $(LOCAL_MODULE) $(my_link_type): PRIVATE_MAKEFILE := $(LOCAL_MODULE_MAKEFILE) -$(my_link_type): $(my_link_type_deps) +$(my_link_type): $(my_link_type_deps) $(CHECK_LINK_TYPE) @echo Check JNI module types: $@ - $(hide) mkdir -p $(dir $@) - $(hide) rm -f $@ - $(hide) for f in $(PRIVATE_DEPS); do \ - grep -qE '^$(PRIVATE_ALLOWED_TYPES)$$' $$f || \ - $(call echo-warning,"$(PRIVATE_MAKEFILE): $(PRIVATE_MODULE) ($(PRIVATE_LINK_TYPE)) should not link to $$(basename $${f%_intermediates/link_type}) ($$(cat $$f))"); \ - done - $(hide) touch $@ + $(check-link-type) $(LOCAL_BUILT_MODULE): | $(my_link_type) diff --git a/core/java_common.mk b/core/java_common.mk index 432c96fc22..1119a378cc 100644 --- a/core/java_common.mk +++ b/core/java_common.mk @@ -379,6 +379,7 @@ endif # need_compile_java ########################################################### ifndef LOCAL_IS_HOST_MODULE my_link_type := $(intermediates.COMMON)/link_type +all_link_types: $(my_link_type) my_link_type_deps := $(strip \ $(foreach lib,$(LOCAL_STATIC_JAVA_LIBRARIES),\ $(call intermediates-dir-for, \ @@ -387,27 +388,25 @@ my_link_type_deps := $(strip \ $(call intermediates-dir-for, \ APPS,$(lib),,COMMON)/link_type)) ifeq ($(LOCAL_SDK_VERSION),system_current) -$(my_link_type): PRIVATE_LINK_TYPE := system -$(my_link_type): PRIVATE_ALLOWED_TYPES := (sdk|system) +$(my_link_type): PRIVATE_LINK_TYPE := java:system +$(my_link_type): PRIVATE_WARN_TYPES := java:platform +$(my_link_type): PRIVATE_ALLOWED_TYPES := java:sdk java:system else ifneq ($(LOCAL_SDK_VERSION),) -$(my_link_type): PRIVATE_LINK_TYPE := sdk -$(my_link_type): PRIVATE_ALLOWED_TYPES := sdk +$(my_link_type): PRIVATE_LINK_TYPE := java:sdk +$(my_link_type): PRIVATE_WARN_TYPES := java:system java:platform +$(my_link_type): PRIVATE_ALLOWED_TYPES := java:sdk else -$(my_link_type): PRIVATE_LINK_TYPE := platform -$(my_link_type): PRIVATE_ALLOWED_TYPES := (sdk|system|platform) +$(my_link_type): PRIVATE_LINK_TYPE := java:platform +$(my_link_type): PRIVATE_WARN_TYPES := +$(my_link_type): PRIVATE_ALLOWED_TYPES := java:sdk java:system java:platform endif +$(eval $(call link-type-partitions,$(my_link_type))) $(my_link_type): PRIVATE_DEPS := $(my_link_type_deps) $(my_link_type): PRIVATE_MODULE := $(LOCAL_MODULE) $(my_link_type): PRIVATE_MAKEFILE := $(LOCAL_MODULE_MAKEFILE) -$(my_link_type): $(my_link_type_deps) +$(my_link_type): $(my_link_type_deps) $(CHECK_LINK_TYPE) @echo Check Java library module types: $@ - $(hide) mkdir -p $(dir $@) - $(hide) rm -f $@ - $(hide) for f in $(PRIVATE_DEPS); do \ - grep -qE '^$(PRIVATE_ALLOWED_TYPES)$$' $$f || \ - $(call echo-warning,"$(PRIVATE_MAKEFILE): $(PRIVATE_MODULE) ($(PRIVATE_LINK_TYPE)) should not link to $$(basename $${f%_intermediates/link_type}) ($$(cat $$f))"); \ - done - $(hide) echo $(PRIVATE_LINK_TYPE) >$@ + $(check-link-type) $(LOCAL_BUILT_MODULE): $(my_link_type) endif # !LOCAL_IS_HOST_MODULE diff --git a/core/main.mk b/core/main.mk index 1eab0db142..b20044ade5 100644 --- a/core/main.mk +++ b/core/main.mk @@ -1137,4 +1137,7 @@ tidy_only: ndk: $(SOONG_OUT_DIR)/ndk.timestamp .PHONY: ndk +.PHONY: all_link_types +all_link_types: + endif # KATI diff --git a/core/prebuilt_internal.mk b/core/prebuilt_internal.mk index 41058ba895..1aa864db12 100644 --- a/core/prebuilt_internal.mk +++ b/core/prebuilt_internal.mk @@ -138,11 +138,12 @@ else endif my_link_type := $(intermediates)/link_type -$(my_link_type): PRIVATE_LINK_TYPE := $(if $(LOCAL_SDK_VERSION),ndk,platform) +$(my_link_type): PRIVATE_LINK_TYPE := native:$(if $(LOCAL_SDK_VERSION),ndk,platform) +$(eval $(call link-type-partitions,$(my_link_type))) $(my_link_type): @echo Check module type: $@ $(hide) mkdir -p $(dir $@) && rm -f $@ - $(hide) echo $(PRIVATE_LINK_TYPE) >$@ + $(hide) echo "$(PRIVATE_LINK_TYPE)" >$@ $(LOCAL_BUILT_MODULE) : | $(export_includes) $(my_link_type) endif # prebuilt_module_is_a_library @@ -397,16 +398,17 @@ $(common_classes_jar) $(common_javalib_jar): PRIVATE_MODULE := $(LOCAL_MODULE) my_link_type := $(intermediates.COMMON)/link_type ifeq ($(LOCAL_SDK_VERSION),system_current) -$(my_link_type): PRIVATE_LINK_TYPE := system +$(my_link_type): PRIVATE_LINK_TYPE := java:system else ifneq ($(LOCAL_SDK_VERSION),) -$(my_link_type): PRIVATE_LINK_TYPE := sdk +$(my_link_type): PRIVATE_LINK_TYPE := java:sdk else -$(my_link_type): PRIVATE_LINK_TYPE := platform +$(my_link_type): PRIVATE_LINK_TYPE := java:platform endif +$(eval $(call link-type-partitions,$(my_link_type))) $(my_link_type): @echo Check module type: $@ $(hide) mkdir -p $(dir $@) && rm -f $@ - $(hide) echo $(PRIVATE_LINK_TYPE) >$@ + $(hide) echo "$(PRIVATE_LINK_TYPE)" >$@ $(LOCAL_BUILT_MODULE): $(my_link_type) ifeq ($(prebuilt_module_is_dex_javalib),true) diff --git a/tools/check_link_type.py b/tools/check_link_type.py new file mode 100755 index 0000000000..40754adc67 --- /dev/null +++ b/tools/check_link_type.py @@ -0,0 +1,80 @@ +#!/usr/bin/env python +# +# Copyright (C) 2016 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. + +"""Utility to verify modules link against acceptable module types""" + +from __future__ import print_function +import argparse +import os +import sys + +WARNING_MSG = ('\033[1m%(makefile)s: \033[35mwarning:\033[0m\033[1m ' + '%(module)s (%(type)s) should not link to %(dep_name)s (%(dep_type)s)' + '\033[0m') +ERROR_MSG = ('\033[1m%(makefile)s: \033[31merror:\033[0m\033[1m ' + '%(module)s (%(type)s) should not link to %(dep_name)s (%(dep_type)s)' + '\033[0m') + +def parse_args(): + """Parse commandline arguments.""" + parser = argparse.ArgumentParser(description='Check link types') + parser.add_argument('--makefile', help='Makefile defining module') + parser.add_argument('--module', help='The module being checked') + parser.add_argument('--type', help='The link type of module') + parser.add_argument('--allowed', help='Allow deps to use these types', + action='append', default=[], metavar='TYPE') + parser.add_argument('--warn', help='Warn if deps use these types', + action='append', default=[], metavar='TYPE') + parser.add_argument('deps', help='The dependencies to check', + metavar='DEP', nargs='*') + return parser.parse_args() + +def print_msg(msg, args, dep_name, dep_type): + """Print a warning or error message""" + print(msg % { + "makefile": args.makefile, + "module": args.module, + "type": args.type, + "dep_name": dep_name, + "dep_type": dep_type}, file=sys.stderr) + +def main(): + """Program entry point.""" + args = parse_args() + + failed = False + for dep in args.deps: + dep_name = os.path.basename(os.path.dirname(dep)) + if dep_name.endswith('_intermediates'): + dep_name = dep_name[:len(dep_name)-len('_intermediates')] + + with open(dep, 'r') as dep_file: + dep_types = dep_file.read().strip().split(' ') + + for dep_type in dep_types: + if dep_type in args.allowed: + continue + if dep_type in args.warn: + print_msg(WARNING_MSG, args, dep_name, dep_type) + else: + print_msg(ERROR_MSG, args, dep_name, dep_type) + failed = True + + if failed: + sys.exit(1) + +if __name__ == '__main__': + main() diff --git a/tools/warn.py b/tools/warn.py index 572ac68dec..5cb104f6cb 100755 --- a/tools/warn.py +++ b/tools/warn.py @@ -91,9 +91,18 @@ warnpatterns = [ 'description':'make: overriding commands/ignoring old commands', 'patterns':[r".*: warning: overriding commands for target .+", r".*: warning: ignoring old commands for target .+"] }, - { 'category':'make', 'severity':severity.HIGH, 'members':[], 'option':'', + { 'category':'make', 'severity':severity.HIGH, 'members':[], 'option':'', 'description':'make: LOCAL_CLANG is false', 'patterns':[r".*: warning: LOCAL_CLANG is set to false"] }, + { 'category':'make', 'severity':severity.HIGH, 'members':[], 'option':'', + 'description':'SDK App using platform shared library', + 'patterns':[r".*: warning: .+ \(.*app:sdk.*\) should not link to .+ \(native:platform\)"] }, + { 'category':'make', 'severity':severity.HIGH, 'members':[], 'option':'', + 'description':'System module linking to a vendor module', + 'patterns':[r".*: warning: .+ \(.+\) should not link to .+ \(partition:.+\)"] }, + { 'category':'make', 'severity':severity.MEDIUM, 'members':[], 'option':'', + 'description':'Invalid SDK/NDK linking', + 'patterns':[r".*: warning: .+ \(.+\) should not link to .+ \(.+\)"] }, { 'category':'C/C++', 'severity':severity.HIGH, 'members':[], 'option':'-Wimplicit-function-declaration', 'description':'Implicit function declaration', 'patterns':[r".*: warning: implicit declaration of function .+",