1 files changed, 135 insertions, 89 deletions

diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go
index 1a0e56f2c7b..e25d4357888 100755
--- a/pkgtools/pkglint/files/options.go
+++ b/pkgtools/pkglint/files/options.go
@@ -1,134 +1,180 @@
 package pkglint
 
 func CheckLinesOptionsMk(mklines MkLines) {
-	if trace.Tracing {
-		defer trace.Call1(mklines.lines.FileName)()
-	}
+	ck := OptionsLinesChecker{
+		mklines,
+		make(map[string]MkLine),
+		make(map[string]MkLine),
+		nil}
+
+	ck.Check()
+}
+
+// OptionsLinesChecker checks an options.mk file of a pkgsrc package.
+//
+// See mk/bsd.options.mk for a detailed description.
+type OptionsLinesChecker struct {
+	mklines MkLines
+
+	declaredOptions           map[string]MkLine
+	handledOptions            map[string]MkLine
+	optionsInDeclarationOrder []string
+}
+
+func (ck *OptionsLinesChecker) Check() {
+	mklines := ck.mklines
 
 	mklines.Check()
 
 	mlex := NewMkLinesLexer(mklines)
 	mlex.SkipWhile(func(mkline MkLine) bool { return mkline.IsComment() || mkline.IsEmpty() })
 
-	if mlex.EOF() || !(mlex.CurrentMkLine().IsVarassign() && mlex.CurrentMkLine().Varname() == "PKG_OPTIONS_VAR") {
-		mlex.CurrentLine().Warnf("Expected definition of PKG_OPTIONS_VAR.")
-		G.Explain(
-			"The input variables in an options.mk file should always be",
-			"mentioned in the same order: PKG_OPTIONS_VAR,",
-			"PKG_SUPPORTED_OPTIONS, PKG_SUGGESTED_OPTIONS.",
-			"This way, the options.mk files have the same structure and are easy to understand.")
+	if !ck.lookingAtPkgOptionsVar(mlex) {
 		return
 	}
 	mlex.Skip()
 
-	declaredOptions := make(map[string]MkLine)
-	handledOptions := make(map[string]MkLine)
-	var optionsInDeclarationOrder []string
+	upper := true
+	for !mlex.EOF() && upper {
+		upper = ck.handleUpperLine(mlex.CurrentMkLine())
+		mlex.Skip()
+	}
+
+	for !mlex.EOF() {
+		ck.handleLowerLine(mlex.CurrentMkLine())
+		mlex.Skip()
+	}
 
-loop:
-	for ; !mlex.EOF(); mlex.Skip() {
+	ck.checkOptionsMismatch()
+
+	mklines.SaveAutofixChanges()
+}
+
+func (ck *OptionsLinesChecker) lookingAtPkgOptionsVar(mlex *MkLinesLexer) bool {
+	if !mlex.EOF() {
 		mkline := mlex.CurrentMkLine()
-		switch {
-		case mkline.IsComment():
-			break
-		case mkline.IsEmpty():
-			break
-
-		case mkline.IsVarassign():
-			switch mkline.Varcanon() {
-			case "PKG_SUPPORTED_OPTIONS", "PKG_OPTIONS_GROUP.*", "PKG_OPTIONS_SET.*":
-				for _, option := range mkline.ValueFields(mkline.Value()) {
-					if !containsVarRef(option) {
-						declaredOptions[option] = mkline
-						optionsInDeclarationOrder = append(optionsInDeclarationOrder, option)
-					}
-				}
-			}
+		if mkline.IsVarassign() && mkline.Varname() == "PKG_OPTIONS_VAR" {
+			return true
+		}
+	}
 
-		case mkline.IsDirective():
-			// The conditionals are typically for OPSYS and MACHINE_ARCH.
+	line := mlex.CurrentLine()
+	line.Warnf("Expected definition of PKG_OPTIONS_VAR.")
+	line.Explain(
+		"The input variables in an options.mk file should always be",
+		"mentioned in the same order: PKG_OPTIONS_VAR,",
+		"PKG_SUPPORTED_OPTIONS, PKG_SUGGESTED_OPTIONS.",
+		"This way, the options.mk files have the same structure and are easy to understand.")
+	return false
+}
 
-		case mkline.IsInclude():
-			if mkline.IncludedFile() == "../../mk/bsd.options.mk" {
-				mlex.Skip()
-				break loop
+// checkLineUpper checks a line from the upper part of an options.mk file,
+// before bsd.options.mk is included.
+func (ck *OptionsLinesChecker) handleUpperLine(mkline MkLine) bool {
+	switch {
+	case mkline.IsComment():
+		break
+	case mkline.IsEmpty():
+		break
+
+	case mkline.IsVarassign():
+		switch mkline.Varcanon() {
+		case "PKG_SUPPORTED_OPTIONS", "PKG_OPTIONS_GROUP.*", "PKG_OPTIONS_SET.*":
+			for _, option := range mkline.ValueFields(mkline.Value()) {
+				if !containsVarRef(option) {
+					ck.declaredOptions[option] = mkline
+					ck.optionsInDeclarationOrder = append(ck.optionsInDeclarationOrder, option)
+				}
 			}
+		}
 
-		default:
-			mlex.CurrentLine().Warnf("Expected inclusion of \"../../mk/bsd.options.mk\".")
-			G.Explain(
-				"After defining the input variables (PKG_OPTIONS_VAR, etc.),",
-				"bsd.options.mk should be included to do the actual processing.",
-				"No other actions should take place in this part of the file",
-				"in order to have the same structure in all options.mk files.")
-			return
+	case mkline.IsDirective():
+		// The conditionals are typically for OPSYS and MACHINE_ARCH.
+
+	case mkline.IsInclude():
+		if mkline.IncludedFile() == "../../mk/bsd.options.mk" {
+			return false
 		}
+
+	default:
+		line := mkline
+		line.Warnf("Expected inclusion of \"../../mk/bsd.options.mk\".")
+		line.Explain(
+			"After defining the input variables (PKG_OPTIONS_VAR, etc.),",
+			"bsd.options.mk should be included to do the actual processing.",
+			"No other actions should take place in this part of the file",
+			"in order to have the same structure in all options.mk files.")
+		return false
 	}
 
-	for ; !mlex.EOF(); mlex.Skip() {
-		mkline := mlex.CurrentMkLine()
-		if mkline.IsDirective() && (mkline.Directive() == "if" || mkline.Directive() == "elif") {
+	return true
+}
+
+func (ck *OptionsLinesChecker) handleLowerLine(mkline MkLine) {
+	if mkline.IsDirective() {
+		directive := mkline.Directive()
+		if directive == "if" || directive == "elif" {
 			cond := mkline.Cond()
-			if cond == nil {
-				continue
+			if cond != nil {
+				ck.handleLowerCondition(mkline, cond)
 			}
+		}
+	}
+}
+
+func (ck *OptionsLinesChecker) handleLowerCondition(mkline MkLine, cond MkCond) {
 
-			recordUsedOption := func(varuse *MkVarUse) {
-				if varuse.varname == "PKG_OPTIONS" && len(varuse.modifiers) == 1 {
-					if m, positive, pattern := varuse.modifiers[0].MatchMatch(); m && positive {
-						option := pattern
-						if !containsVarRef(option) {
-							handledOptions[option] = mkline
-							optionsInDeclarationOrder = append(optionsInDeclarationOrder, option)
-						}
-					}
+	recordUsedOption := func(varuse *MkVarUse) {
+		if varuse.varname == "PKG_OPTIONS" && len(varuse.modifiers) == 1 {
+			if m, positive, pattern := varuse.modifiers[0].MatchMatch(); m && positive {
+				option := pattern
+				if !containsVarRef(option) {
+					ck.handledOptions[option] = mkline
+					ck.optionsInDeclarationOrder = append(ck.optionsInDeclarationOrder, option)
 				}
 			}
-			cond.Walk(&MkCondCallback{
-				Empty: recordUsedOption,
-				Var:   recordUsedOption})
-
-			// FIXME: Is this note also issued for the following lines?
-			//  .if empty(ANY_OTHER_VARIABLE)
-			//  .else
-			//  .endif
-			if cond.Empty != nil && mkline.HasElseBranch() {
-				mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.")
-				G.Explain(
-					"For consistency among packages, the upper branch of this",
-					".if/.else statement should always handle the case where the",
-					"option is activated.",
-					"A missing exclamation mark at this point can easily be overlooked.",
-					"",
-					"If that seems too much to type and the exclamation mark",
-					"seems wrong for a positive test, switch the blocks nevertheless",
-					"and write the condition like this, which has the same effect",
-					"as the !empty(...).",
-					"",
-					"\t.if ${PKG_OPTIONS.packagename:Moption}")
-			}
 		}
 	}
 
-	for _, option := range optionsInDeclarationOrder {
-		declared := declaredOptions[option]
-		handled := handledOptions[option]
+	cond.Walk(&MkCondCallback{
+		Empty: recordUsedOption,
+		Var:   recordUsedOption})
+
+	if cond.Empty != nil && cond.Empty.varname == "PKG_OPTIONS" && mkline.HasElseBranch() {
+		mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.")
+		mkline.Explain(
+			"For consistency among packages, the upper branch of this",
+			".if/.else statement should always handle the case where the",
+			"option is activated.",
+			"A missing exclamation mark at this point can easily be overlooked.",
+			"",
+			"If that seems too much to type and the exclamation mark",
+			"seems wrong for a positive test, switch the blocks nevertheless",
+			"and write the condition like this, which has the same effect",
+			"as the !empty(...).",
+			"",
+			"\t.if ${PKG_OPTIONS.packagename:Moption}")
+	}
+}
+
+func (ck *OptionsLinesChecker) checkOptionsMismatch() {
+	for _, option := range ck.optionsInDeclarationOrder {
+		declared := ck.declaredOptions[option]
+		handled := ck.handledOptions[option]
 
 		switch {
 		case handled == nil:
 			declared.Warnf("Option %q should be handled below in an .if block.", option)
-			G.Explain(
+			declared.Explain(
 				"If an option is not processed in this file, it may either be a",
 				"typo, or the option does not have any effect.")
 
 		case declared == nil:
 			handled.Warnf("Option %q is handled but not added to PKG_SUPPORTED_OPTIONS.", option)
-			G.Explain(
+			handled.Explain(
 				"This block of code will never be run since PKG_OPTIONS cannot",
 				"contain this value.",
 				"This is most probably a typo.")
 		}
 	}
-
-	mklines.SaveAutofixChanges()
 }