From aaa8fc399837eecccbc98e4d29b5231536ebb77c Mon Sep 17 00:00:00 2001 From: Zach Shepherd Date: Thu, 20 Sep 2018 17:39:51 -0700 Subject: [PATCH] Warn about unused rules --- Makefile | 3 +- infra/scripts/go-deps-enforcement.sh | 78 +++++++++++++++++-- lib/apiservers/engine/.godeps_rules | 8 +- .../engine/backends/middleware/.godeps_rules | 12 +-- lib/apiservers/portlayer/client/.godeps_rules | 2 +- .../restapi/operations/.godeps_rules | 2 +- .../service/restapi/operations/.godeps_rules | 2 +- lib/constants/.godeps_rules | 2 +- lib/install/management/.godeps_rules | 4 +- lib/portlayer/storage/.godeps_rules | 4 +- pkg/version/.godeps_rules | 4 +- 11 files changed, 93 insertions(+), 28 deletions(-) diff --git a/Makefile b/Makefile index 25e246a08a..a004b2179a 100644 --- a/Makefile +++ b/Makefile @@ -240,7 +240,8 @@ govet: gas: $(GAS) @echo checking security problems -enforce-deps: +# requires swagger generation to avoid warnings about unused rules +enforce-deps: $(portlayerapi-client) $(portlayerapi-server) $(serviceapi-server) @echo checking dependencies... @infra/scripts/go-deps-enforcement.sh diff --git a/infra/scripts/go-deps-enforcement.sh b/infra/scripts/go-deps-enforcement.sh index 396e08488f..bf4d65098c 100755 --- a/infra/scripts/go-deps-enforcement.sh +++ b/infra/scripts/go-deps-enforcement.sh @@ -29,6 +29,17 @@ find-packages () { find "${@}" -type f -name '*.go' -exec dirname {} \; | sort --unique } +# Returns a list of all rules files +# Arguments +# *: directories to search under +# +# Returns: +# rule files within searched directories +find-rules () { + find "${@}" -type f -name "$RULES_FILE" | sort --unique +} + + # Returns the path to the "nearest" rule file for a given package # Arguments # 1: package path @@ -36,7 +47,7 @@ find-packages () { # Returns: # path to rule file (defaulting to /dev/null) find-rule () { - path="${1?Package path must be provided}" + local path="${1?Package path must be provided}" shift while [[ "$path" != "." ]]; do @@ -58,7 +69,7 @@ find-rule () { # Returns: # contents of the file, excluding blank lines and lines beginning with "#" get-rules () { - rules="${1?Rules file must be provided}" + local rules="${1?Rules file must be provided}" shift grep -v -e '^$' -e '^#' "$rules" @@ -71,7 +82,7 @@ get-rules () { # Returns: # a list of direct and transitive dependencies get-deps () { - package="${1?Package must be provided}" + local package="${1?Package must be provided}" shift infra/scripts/go-deps.sh "$package" @@ -86,23 +97,76 @@ get-deps () { # Returns: # a list of invalid dependencies filter-deps () { - package="${1?Package must be provided}" - rules="${2?Rules file must be provided}" + local package="${1?Package must be provided}" + local rules="${2?Rules file must be provided}" shift 2 get-deps "$package" | grep -v -e "^$package/*" -f <(get-rules "$rules") || true } +# Returns any unused rules for a given set of packages (does not identify duplicate rules!) +# Arguments +# 1: the dependencies +# 2: path to rules file +# +# Returns: +# a list of rules which match none of the dependencies +filter-rules () { + local deps="${1?Dependencies must be provided}" + local rules="${2?Rules file must be provided}" + shift 2 + + local unmatched=() + for rule in $(get-rules "$rules"); do + matches=$(echo "$deps" | grep -c -e "$rule" || true) + + if [ "$matches" = "0" ]; then + unmatched+=("$rule") + fi + done + + echo "${unmatched[*]}" +} + + rc=0 + for package in $(find-packages "${ENFORCE[@]}"); do rules="$(find-rule "$package")" + invalid=$(filter-deps "$package" "$rules") + if [ ! -z "$invalid" ]; then echo "Unexpected dependencies in $package:" - echo "${invalid//^/ /}" + echo "$invalid" | sed -e 's/^/ /' echo "See $rules for details." echo "" - rc=1 + + rc=$(( rc | 1 )) + fi +done + +export VIC_CACHE_DEPS=true # We've just calculated all of these; no need to do so again + +for rules in $(find-rules "${ENFORCE[@]}"); do + directory=$(dirname "$rules") + + deps="" + for package in $(find-packages "$directory"); do + if [ "$rules" != "$(find-rule "$package")" ]; then + continue # This package is using a more specific rules file + fi + deps+="$(get-deps "$package")" + done + + unused="$(filter-rules "$deps" "$rules")" + + if [ ! -z "$unused" ]; then + echo "Unused rules in $rules:" + echo "$unused" | sed -e 's/^/ /' + echo "" + + rc=$(( rc | 2 )) fi done diff --git a/lib/apiservers/engine/.godeps_rules b/lib/apiservers/engine/.godeps_rules index fb3bd829ca..d272a53759 100644 --- a/lib/apiservers/engine/.godeps_rules +++ b/lib/apiservers/engine/.godeps_rules @@ -18,15 +18,15 @@ ^lib/config/* ^lib/constants/* ^lib/imagec/* -^lib/iolog/* +#^lib/iolog/* ^lib/metadata/* ^lib/migration/* ^lib/spec/* -^lib/tether/shared/* +#^lib/tether/shared/* # lib/tether/shared also depends on lib/system -^lib/system/* +#^lib/system/* # lib/system also depends on lib/etcconf -^lib/etcconf/* +#^lib/etcconf/* diff --git a/lib/apiservers/engine/backends/middleware/.godeps_rules b/lib/apiservers/engine/backends/middleware/.godeps_rules index 6ee90d46b0..59d73c7c88 100644 --- a/lib/apiservers/engine/backends/middleware/.godeps_rules +++ b/lib/apiservers/engine/backends/middleware/.godeps_rules @@ -3,23 +3,23 @@ ^vendor/* # lib/apiservers/engine subpackages can depend on other subpackages -^lib/apiservers/engine/* +#^lib/apiservers/engine/* # lib/apiservers/engine acts as a client of the portlayer API -^lib/apiservers/portlayer/client/* -^lib/apiservers/portlayer/models/* +#^lib/apiservers/portlayer/client/* +#^lib/apiservers/portlayer/models/* # lib/apiservers/engine also depends on portlayer utilities and events ^lib/portlayer/util/* ^lib/portlayer/event/events/* # lib/apiservers/engine also depends on other lib packages -^lib/archive/* +#^lib/archive/* ^lib/config/* ^lib/constants/* -^lib/imagec/* +#^lib/imagec/* ^lib/iolog/* -^lib/metadata/* +#^lib/metadata/* ^lib/migration/* ^lib/spec/* ^lib/tether/shared/* diff --git a/lib/apiservers/portlayer/client/.godeps_rules b/lib/apiservers/portlayer/client/.godeps_rules index 91b38bacbe..58bcfb70c0 100644 --- a/lib/apiservers/portlayer/client/.godeps_rules +++ b/lib/apiservers/portlayer/client/.godeps_rules @@ -1,5 +1,5 @@ # By default, packages in lib should depend only on packages under pkg or vendor -^pkg/* +#^pkg/* ^vendor/* # portlayer API client may reference portlayer API model objects diff --git a/lib/apiservers/portlayer/restapi/operations/.godeps_rules b/lib/apiservers/portlayer/restapi/operations/.godeps_rules index c2458b2279..4ba4fd8fb6 100644 --- a/lib/apiservers/portlayer/restapi/operations/.godeps_rules +++ b/lib/apiservers/portlayer/restapi/operations/.godeps_rules @@ -1,5 +1,5 @@ # By default, packages in lib should depend only on packages under pkg or vendor -^pkg/* +#^pkg/* ^vendor/* # portlayer API operations may reference portlayer API model objects diff --git a/lib/apiservers/service/restapi/operations/.godeps_rules b/lib/apiservers/service/restapi/operations/.godeps_rules index be4fd7916c..dae8b11df1 100644 --- a/lib/apiservers/service/restapi/operations/.godeps_rules +++ b/lib/apiservers/service/restapi/operations/.godeps_rules @@ -1,5 +1,5 @@ # By default, packages in lib should depend only on packages under pkg or vendor -^pkg/* +#^pkg/* ^vendor/* # service API operations may reference service API model objects diff --git a/lib/constants/.godeps_rules b/lib/constants/.godeps_rules index 878a8d3eec..0f6f6a97a8 100644 --- a/lib/constants/.godeps_rules +++ b/lib/constants/.godeps_rules @@ -1,6 +1,6 @@ # By default, packages in lib should depend only on packages under pkg or vendor ^pkg/* -^vendor/* +#^vendor/* # BAD: lib/constants depends on pkg/version, which depends on lib/migration/feature ^lib/migration/feature/* diff --git a/lib/install/management/.godeps_rules b/lib/install/management/.godeps_rules index 9d33d3ab23..af0928b8c8 100644 --- a/lib/install/management/.godeps_rules +++ b/lib/install/management/.godeps_rules @@ -21,10 +21,10 @@ ^lib/install/vchlog/* # QUESTIONABLE: lib/install/management also depends on portlayer utilities -lib/portlayer/util/* +^lib/portlayer/util/* # QUESTIONABLE: lib/install/management also depends on the shared tether sub-package -lib/tether/shared/* +^lib/tether/shared/* # BAD: lib/install/management also depends on code from cmd/vic-machine/common, but it should not ^cmd/vic-machine/common/* diff --git a/lib/portlayer/storage/.godeps_rules b/lib/portlayer/storage/.godeps_rules index 1f0e1f1f4d..fdcbc3a0b7 100644 --- a/lib/portlayer/storage/.godeps_rules +++ b/lib/portlayer/storage/.godeps_rules @@ -10,7 +10,7 @@ ^lib/iolog/* ^lib/migration/* ^lib/spec/* -^lib/tether/msgs/* +#^lib/tether/msgs/* ^lib/tether/shared/* # lib/tether/shared also depends on lib/system @@ -23,7 +23,7 @@ # event, exec, store, and util are valid dependencies for other sub-packages ^lib/portlayer/event/* ^lib/portlayer/exec/* -^lib/portlayer/store/* +#^lib/portlayer/store/* ^lib/portlayer/util/* # lib/portlayer/storage subpackages can depend on lib/portlayer/storage diff --git a/pkg/version/.godeps_rules b/pkg/version/.godeps_rules index b9c250524e..a83cd72e31 100644 --- a/pkg/version/.godeps_rules +++ b/pkg/version/.godeps_rules @@ -1,6 +1,6 @@ # By default, packages in pkg should depend only on packages under pkg or vendor -^pkg/* -^vendor/* +#^pkg/* +#^vendor/* # BAD: pkg/version/version.go currently depends on lib/migration/feature, but should not ^lib/migration/feature/*