[mynewt-newt] branch master updated: Discard duplicates in merged dep conditional

[cc...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

ccollins pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mynewt-newt.git


The following commit(s) were added to refs/heads/master by this push:
     new 430b0bb  Discard duplicates in merged dep conditional
430b0bb is described below

commit 430b0bb91da119e67a8111f772ef9b287426ce58
Author: Christopher Collins <cc...@apache.org>
AuthorDate: Mon Nov 26 13:27:12 2018 -0800

    Discard duplicates in merged dep conditional
    
    Newt merges conditional dependencies into a single dependency.  For
    example, newt transforms the following:
    
        pkg.deps.SETTING_A:
            - my_package
    
        pkg.deps.SETTING_B:
            - my_package
    
    into:
    
        pkg.deps.'(SETTING_A || SETTING B):
            - my_package
    
    (internally)
    
    This allows a `pkg.yml` file to declare separate conditions under which
    a package gets pulled in, rather than requiring the user to manually
    collapse them all into a single expression.
    
    The bug is that sometimes newt would add duplicate expressions to the
    merged expression, e.g.,
    
        pkg.deps.'((SETTING_A || SETTING_B) || SETTING_B)'
    
    The resulting dependency graph is unchanged, but the text in the log and
    in the dep / revdep commands was ugly and confusing.
    
    The problem was that newt was not always normalizing each expression
    before comparing it to the previously-seen expressions.  If an
    expression contained extra whitespace, it would not compare equal to the
    equivalent whitespace-free expression. This commit fixes this issue by
    always normalizing conditionals before storing them.
---
 newt/resolve/resolve.go | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/newt/resolve/resolve.go b/newt/resolve/resolve.go
index 46e6294..c9ccb42 100644
--- a/newt/resolve/resolve.go
+++ b/newt/resolve/resolve.go
@@ -234,6 +234,12 @@ func (r *Resolver) resolveDep(dep *pkg.Dependency,
 func (rpkg *ResolvePackage) AddDep(
 	depPkg *ResolvePackage, api string, expr string) bool {
 
+	norm, err := parse.NormalizeExpr(expr)
+	if err != nil {
+		panic("invalid expression, should have been caught earlier: " +
+			err.Error())
+	}
+
 	if dep := rpkg.Deps[depPkg]; dep != nil {
 		// This package already depends on dep.  If the conditional expression
 		// is new, or if the API string is different, then the existing
@@ -243,18 +249,11 @@ func (rpkg *ResolvePackage) AddDep(
 		// Determine if this is a new conditional expression.
 		oldExpr := dep.ExprString()
 
-		norm, err := parse.NormalizeExpr(expr)
-		if err != nil {
-			panic("invalid expression, should have been caught earlier: " +
-				err.Error())
-		}
-
-		dep.ExprMap[norm] = struct{}{}
-		merged := dep.ExprString()
-
 		changed := false
+		if _, ok := dep.ExprMap[norm]; !ok {
+			dep.ExprMap[norm] = struct{}{}
+			merged := dep.ExprString()
 
-		if oldExpr != merged {
 			log.Debugf("Package %s has conflicting dependencies on %s: "+
 				"old=`%s` new=`%s`; merging them into a single conditional: "+
 				"`%s`",
@@ -274,7 +273,7 @@ func (rpkg *ResolvePackage) AddDep(
 			Rpkg: depPkg,
 			Api:  api,
 			ExprMap: map[string]struct{}{
-				expr: struct{}{},
+				norm: struct{}{},
 			},
 		}
 	}