Skip to content

Commit

Permalink
Warn about unused rules
Browse files Browse the repository at this point in the history
  • Loading branch information
zjs committed Sep 22, 2018
1 parent e182fb2 commit aaa8fc3
Show file tree
Hide file tree
Showing 11 changed files with 93 additions and 28 deletions.
3 changes: 2 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
78 changes: 71 additions & 7 deletions infra/scripts/go-deps-enforcement.sh
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,25 @@ 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
#
# 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
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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

Expand Down
8 changes: 4 additions & 4 deletions lib/apiservers/engine/.godeps_rules
Original file line number Diff line number Diff line change
Expand Up @@ -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/*

12 changes: 6 additions & 6 deletions lib/apiservers/engine/backends/middleware/.godeps_rules
Original file line number Diff line number Diff line change
Expand Up @@ -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/*
Expand Down
2 changes: 1 addition & 1 deletion lib/apiservers/portlayer/client/.godeps_rules
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/apiservers/portlayer/restapi/operations/.godeps_rules
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/apiservers/service/restapi/operations/.godeps_rules
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/constants/.godeps_rules
Original file line number Diff line number Diff line change
@@ -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/*
Expand Down
4 changes: 2 additions & 2 deletions lib/install/management/.godeps_rules
Original file line number Diff line number Diff line change
Expand Up @@ -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/*
4 changes: 2 additions & 2 deletions lib/portlayer/storage/.godeps_rules
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/version/.godeps_rules
Original file line number Diff line number Diff line change
@@ -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/*
Expand Down

0 comments on commit aaa8fc3

Please sign in to comment.