From 1d8b4fe0eb25c89fd159d3654cdc175c7909b7bf Mon Sep 17 00:00:00 2001 From: rillig Date: Sat, 14 Dec 2019 18:04:15 +0000 Subject: pkgtools/pkglint: update to 19.3.18 Changes since 19.3.17: The SUBST check has been completely rewritten. It can handle several SUBST classes at the same time now. This reduces the number of wrong warnings. --- pkgtools/pkglint/Makefile | 5 +- pkgtools/pkglint/files/mkcondchecker_test.go | 3 +- pkgtools/pkglint/files/substcontext.go | 793 ++++++++------ pkgtools/pkglint/files/substcontext_test.go | 1438 +++++++++++++++----------- pkgtools/pkglint/files/vardefs.go | 4 +- pkgtools/pkglint/files/vartype.go | 5 - pkgtools/pkglint/files/vartypecheck.go | 8 +- pkgtools/pkglint/files/vartypecheck_test.go | 12 +- 8 files changed, 1303 insertions(+), 965 deletions(-) (limited to 'pkgtools') diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index d4e092b8e6a..9bbdcac99b6 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,7 +1,6 @@ -# $NetBSD: Makefile,v 1.618 2019/12/13 07:44:02 bsiegert Exp $ +# $NetBSD: Makefile,v 1.619 2019/12/14 18:04:15 rillig Exp $ -PKGNAME= pkglint-19.3.17 -PKGREVISION= 1 +PKGNAME= pkglint-19.3.18 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go index 6b55aaaa898..062897874f5 100644 --- a/pkgtools/pkglint/files/mkcondchecker_test.go +++ b/pkgtools/pkglint/files/mkcondchecker_test.go @@ -73,8 +73,7 @@ func (s *Suite) Test_MkCondChecker_Check(c *check.C) { test(".if ${MACHINE_PLATFORM:MUnknownOS-*-*} || ${MACHINE_ARCH:Mx86}", "WARN: filename.mk:4: "+ "The pattern \"UnknownOS\" cannot match any of "+ - "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ - "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+ "} for the operating system part of MACHINE_PLATFORM.", "WARN: filename.mk:4: "+ "The pattern \"x86\" cannot match any of "+ diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index 49aa13296c0..1ee397e665f 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -3,30 +3,25 @@ package pkglint import "netbsd.org/pkglint/textproc" // SubstContext records the state of a block of variable assignments -// that make up a SUBST class (see `mk/subst.mk`). +// that make up a SUBST class. +// +// See mk/subst.mk. type SubstContext struct { - queuedIds []string - id string - doneIds map[string]bool + active *substBlock - foreignAllowed map[string]struct{} - foreign []*MkLine - - conds []*substCond + scopes []*substScope once Once } func NewSubstContext() *SubstContext { - ctx := SubstContext{} - ctx.reset() - return &ctx + return &SubstContext{scopes: []*substScope{newSubstScope()}} } func (ctx *SubstContext) Process(mkline *MkLine) { switch { case mkline.IsEmpty(): - ctx.finishClass(mkline) + ctx.emptyLine() case mkline.IsVarassign(): ctx.varassign(mkline) case mkline.IsDirective(): @@ -35,8 +30,20 @@ func (ctx *SubstContext) Process(mkline *MkLine) { } func (ctx *SubstContext) Finish(diag Diagnoser) { - ctx.finishClass(diag) - ctx.finishFile(diag) + // Prevent panics on unbalanced conditionals. + for len(ctx.scopes) > 1 { + ctx.leave(diag) + } + + for _, scope := range ctx.scopes { + scope.finish(diag) + } +} + +func (ctx *SubstContext) emptyLine() { + for _, scope := range ctx.scopes { + scope.emptyLine() + } } func (ctx *SubstContext) varassign(mkline *MkLine) { @@ -47,8 +54,8 @@ func (ctx *SubstContext) varassign(mkline *MkLine) { } if ctx.isForeign(mkline.Varcanon()) { - if ctx.isActive() { - ctx.rememberForeign(mkline) + if ctx.isActive() && !ctx.block().seenEmpty { + ctx.block().rememberForeign(mkline) } return } @@ -59,115 +66,341 @@ func (ctx *SubstContext) varassign(mkline *MkLine) { } } - if hasPrefix(mkline.Varname(), "SUBST_") && !ctx.isActiveId(mkline.Varparam()) { + if hasPrefix(mkline.Varname(), "SUBST_") && mkline.Varparam() != ctx.activeId() { if !ctx.varassignDifferentClass(mkline) { return } } + block := ctx.block() switch varcanon { case "SUBST_STAGE.*": - ctx.varassignStage(mkline) + block.varassignStage(mkline) case "SUBST_MESSAGE.*": - ctx.varassignMessages(mkline) + block.varassignMessages(mkline) case "SUBST_FILES.*": - ctx.varassignFiles(mkline) + block.varassignFiles(mkline) case "SUBST_SED.*": - ctx.varassignSed(mkline) + block.varassignSed(mkline) case "SUBST_VARS.*": - ctx.varassignVars(mkline) + block.varassignVars(mkline) case "SUBST_FILTER_CMD.*": - ctx.varassignFilterCmd(mkline) + block.varassignFilterCmd(mkline) } } func (ctx *SubstContext) varassignClasses(mkline *MkLine) { - classes := mkline.ValueFields(mkline.WithoutMakeVariables(mkline.Value())) - if len(classes) == 0 { + ids := mkline.ValueFields(mkline.WithoutMakeVariables(mkline.Value())) + if len(ids) == 0 { return } - if len(classes) > 1 { + if len(ids) > 1 { mkline.Notef("Please add only one class at a time to SUBST_CLASSES.") mkline.Explain( "This way, each substitution class forms a block in the package Makefile,", "and to delete this block, it is not necessary to look anywhere else.") } - for _, class := range classes { - ctx.queue(class) - } - id := classes[0] - if ctx.isActive() && !ctx.isActiveId(id) { - id := ctx.activeId() // since ctx.condEndif may reset it + ctx.prepareSubstClasses(mkline) + ctx.deactivate(mkline) - for ctx.isConditional() { - // This will be confusing for the outer SUBST block, - // but since that block is assumed to be finished, - // this doesn't matter. - ctx.condEndif(mkline) - } - - complete := ctx.isComplete() // since ctx.finishClass will reset it - ctx.finishClass(mkline) - if !complete { - mkline.Warnf("Subst block %q should be finished before adding the next class to SUBST_CLASSES.", id) + for _, id := range ids { + if ctx.lookup(id) == nil { + ctx.scopes[len(ctx.scopes)-1].define(id) + } else if mkline.Varparam() == "" { + mkline.Errorf("Duplicate SUBST class %q.", id) } } +} - ctx.setActiveId(id) - - return +func (ctx *SubstContext) prepareSubstClasses(mkline *MkLine) { + for _, scope := range ctx.scopes { + scope.prepareSubstClasses(mkline) + } } // varassignOutsideBlock handles variable assignments of SUBST variables that // appear without a directly corresponding SUBST block. func (ctx *SubstContext) varassignOutsideBlock(mkline *MkLine) (continueWithNewId bool) { - varparam := mkline.Varparam() + id := mkline.Varparam() - if ctx.isListCanon(mkline.Varcanon()) && ctx.isDone(varparam) { + if id != "" && ctx.isListCanon(mkline.Varcanon()) && ctx.isDone(id) { if mkline.Op() != opAssignAppend { mkline.Warnf("Late additions to a SUBST variable should use the += operator.") } return } - if containsWord(mkline.Rationale(), varparam) { - return + + return ctx.activate(mkline, ctx.lookup(id) == nil) +} + +func (ctx *SubstContext) varassignDifferentClass(mkline *MkLine) (ok bool) { + varname := mkline.Varname() + unknown := ctx.lookup(mkline.Varparam()) == nil + if unknown && ctx.isActive() && !ctx.block().isComplete() { + mkline.Warnf("Variable %q does not match SUBST class %q.", + varname, ctx.activeId()) + if !ctx.block().seenEmpty { + return false + } + } + + return ctx.activate(mkline, unknown) +} + +func (ctx *SubstContext) directive(mkline *MkLine) { + dir := mkline.Directive() + switch dir { + case "if": + ctx.enter() + case "elif": + ctx.nextBranch(mkline, false) + case "else": + ctx.nextBranch(mkline, true) + case "endif": + ctx.leave(mkline) + } +} + +func (ctx *SubstContext) enter() { + for _, scope := range ctx.scopes { + scope.enter() + } + ctx.scopes = append(ctx.scopes, newSubstScope()) +} + +func (ctx *SubstContext) nextBranch(diag Diagnoser, isElse bool) { + if ctx.isActive() && !ctx.block().isConditional() { + ctx.deactivate(diag) + } + + for _, scope := range ctx.scopes { + scope.nextBranch(diag, isElse) + } +} + +func (ctx *SubstContext) leave(diag Diagnoser) { + ctx.deactivate(diag) + + for _, scope := range ctx.scopes { + scope.leave(diag) } - if ctx.start(varparam) { + if len(ctx.scopes) > 1 { + ctx.scopes = ctx.scopes[:len(ctx.scopes)-1] + } +} + +func (ctx *SubstContext) activate(mkline *MkLine, deactivate bool) bool { + id := mkline.Varparam() + if id == "" { + mkline.Errorf("Invalid SUBST class %q in variable name.", id) + return false + } + + if deactivate { + ctx.deactivate(mkline) + } + + if block := ctx.lookup(id); block != nil { + ctx.active = block return true } - if ctx.once.FirstTime(varparam) { + if ctx.once.FirstTime(id) && !containsWord(mkline.Rationale(), id) { mkline.Warnf("Before defining %s, the SUBST class "+ "should be declared using \"SUBST_CLASSES+= %s\".", - mkline.Varname(), varparam) + mkline.Varname(), id) } - return + return false } -func (ctx *SubstContext) varassignDifferentClass(mkline *MkLine) (ok bool) { - varname := mkline.Varname() - varparam := mkline.Varparam() +func (ctx *SubstContext) deactivate(diag Diagnoser) { + if !ctx.isActive() { + return + } - if !ctx.isComplete() { - mkline.Warnf("Variable %q does not match SUBST class %q.", varname, ctx.activeId()) + block := ctx.block() + if !block.isConditional() { + block.finish(diag) + } + ctx.active = nil +} + +func (*SubstContext) isForeign(varcanon string) bool { + switch varcanon { + case + "SUBST_STAGE.*", + "SUBST_MESSAGE.*", + "SUBST_FILES.*", + "SUBST_SED.*", + "SUBST_VARS.*", + "SUBST_FILTER_CMD.*": return false } + return true +} + +func (*SubstContext) isListCanon(varcanon string) bool { + switch varcanon { + case + "SUBST_FILES.*", + "SUBST_SED.*", + "SUBST_VARS.*": + return true + } + return false +} - ctx.finishClass(mkline) +func (ctx *SubstContext) block() *substBlock { + assertNotNil(ctx.active) + return ctx.active +} - ctx.start(varparam) - return true +func (ctx *SubstContext) lookup(id string) *substBlock { + for i := len(ctx.scopes) - 1; i >= 0; i-- { + if def := ctx.scopes[i].def(id); def != nil { + return def + } + } + return nil +} + +func (ctx *SubstContext) isDone(id string) bool { + for _, scope := range ctx.scopes { + if scope.isDone(id) { + return true + } + } + return false +} + +func (ctx *SubstContext) isActive() bool { return ctx.active != nil } + +func (ctx *SubstContext) activeId() string { + assert(ctx.isActive()) + return ctx.active.id +} + +type substScope struct { + defs []*substBlock +} + +func newSubstScope() *substScope { + return &substScope{nil} +} + +func (s *substScope) def(id string) *substBlock { + for _, def := range s.defs { + if def.id == id { + return def + } + } + return nil +} + +func (s *substScope) define(id string) { + assert(s.def(id) == nil) + s.defs = append(s.defs, newSubstBlock(id)) +} + +func (s *substScope) isDone(id string) bool { + def := s.def(id) + return def != nil && def.done } -func (ctx *SubstContext) varassignStage(mkline *MkLine) { - if ctx.isConditional() { +func (s *substScope) emptyLine() { + for _, block := range s.defs { + block.seenEmpty = true + } +} + +// finish brings all blocks that are defined in the current scope +// to an end. +func (s *substScope) finish(diag Diagnoser) { + for _, block := range s.defs { + block.finish(diag) + } +} + +func (s *substScope) prepareSubstClasses(diag Diagnoser) { + for _, block := range s.defs { + if block.hasStarted() && !block.isComplete() { + diag.Warnf("Subst block %q should be finished "+ + "before adding the next class to SUBST_CLASSES.", + block.id) + } + } +} + +func (s *substScope) enter() { + for _, block := range s.defs { + block.enter() + } +} + +func (s *substScope) nextBranch(diag Diagnoser, isElse bool) { + for _, block := range s.defs { + if block.isConditional() { + block.nextBranch(isElse) + } else { + block.finish(diag) + } + } +} + +func (s *substScope) leave(diag Diagnoser) { + for _, block := range s.defs { + if block.isConditional() { + block.leave() + } else { + block.finish(diag) + } + } +} + +type substBlock struct { + id string + + // Records which of the SUBST variables have been seen either + // directly or in a conditional branch. + conds []*substCond + + // In the paragraph of a SUBST block, there should be only variables + // that actually belong to the SUBST block. + // + // In addition, variables that are mentioned in SUBST_VARS may also + // be defined there because they closely relate to the SUBST block. + foreignAllowed map[string]struct{} + foreign []*MkLine + + // Whether there has been an empty line between the SUBST_CLASSES + // line and the current line. + // + // Before the empty line, variables that don't obviously belong to + // this SUBST block generate warnings since they may be typos, + // such as for a different SUBST block. + seenEmpty bool + + // Whether the SUBST_CLASSES has already gone out of scope. + // + // XXX: When it is out of scope, it should also be unreachable + // by any pkglint code. There's something inconsistent here. + done bool +} + +func newSubstBlock(id string) *substBlock { + assert(id != "") + return &substBlock{id: id, conds: []*substCond{newSubstCond()}} +} + +func (b *substBlock) varassignStage(mkline *MkLine) { + if b.isConditional() { mkline.Warnf("%s should not be defined conditionally.", mkline.Varname()) } - ctx.dupString(mkline, ssStage) + b.dupString(mkline, ssStage) value := mkline.Value() if value == "pre-patch" || value == "post-patch" { @@ -196,84 +429,55 @@ func (ctx *SubstContext) varassignStage(mkline *MkLine) { } } -func (ctx *SubstContext) varassignMessages(mkline *MkLine) { +func (b *substBlock) varassignMessages(mkline *MkLine) { varname := mkline.Varname() - if ctx.isConditional() { + if b.isConditional() { mkline.Warnf("%s should not be defined conditionally.", varname) } - ctx.dupString(mkline, ssMessage) + b.dupString(mkline, ssMessage) } -func (ctx *SubstContext) varassignFiles(mkline *MkLine) { - ctx.dupList(mkline, ssFiles, ssNone) +func (b *substBlock) varassignFiles(mkline *MkLine) { + b.dupList(mkline, ssFiles, ssNone) } -func (ctx *SubstContext) varassignSed(mkline *MkLine) { - ctx.dupList(mkline, ssSed, ssNone) - ctx.seen().set(ssTransform) +func (b *substBlock) varassignSed(mkline *MkLine) { + b.dupList(mkline, ssSed, ssNone) + b.addSeen(ssTransform) - ctx.suggestSubstVars(mkline) + b.suggestSubstVars(mkline) } -func (ctx *SubstContext) varassignVars(mkline *MkLine) { - ctx.dupList(mkline, ssVars, ssVarsAutofix) - ctx.seen().set(ssTransform) +func (b *substBlock) varassignVars(mkline *MkLine) { + b.dupList(mkline, ssVars, ssVarsAutofix) + b.addSeen(ssTransform) for _, substVar := range mkline.Fields() { - ctx.allowVar(substVar) + b.allowVar(substVar) } } -func (ctx *SubstContext) varassignFilterCmd(mkline *MkLine) { - ctx.dupString(mkline, ssFilterCmd) - ctx.seen().set(ssTransform) +func (b *substBlock) varassignFilterCmd(mkline *MkLine) { + b.dupString(mkline, ssFilterCmd) + b.addSeen(ssTransform) } -func (ctx *SubstContext) dupList(mkline *MkLine, part substSeen, autofixPart substSeen) { - if ctx.seenInBranch(part) && mkline.Op() != opAssignAppend { - ctx.fixOperatorAppend(mkline, ctx.seenInBranch(autofixPart)) - } - ctx.seen().set(part) -} - -func (ctx *SubstContext) dupString(mkline *MkLine, part substSeen) { - if ctx.seenInBranch(part) { - mkline.Warnf("Duplicate definition of %q.", mkline.Varname()) - } - ctx.seen().set(part) -} - -func (ctx *SubstContext) fixOperatorAppend(mkline *MkLine, dueToAutofix bool) { - before := mkline.ValueAlign() - after := alignWith(mkline.Varname()+"+=", before) - - fix := mkline.Autofix() - if dueToAutofix { - fix.Notef(SilentAutofixFormat) - } else { - fix.Warnf("All but the first assignment to %q should use the \"+=\" operator.", - mkline.Varname()) - } - fix.Replace(before, after) - fix.Apply() -} - -func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) { +func (b *substBlock) suggestSubstVars(mkline *MkLine) { tokens, _ := splitIntoShellTokens(mkline.Line, mkline.Value()) for _, token := range tokens { - varname := ctx.extractVarname(mkline.UnquoteShell(token, false)) + varname := b.extractVarname(mkline.UnquoteShell(token, false)) if varname == "" { continue } - id := ctx.activeId() + id := b.id varop := sprintf("SUBST_VARS.%s%s%s", id, condStr(hasSuffix(id, "+"), " ", ""), - condStr(ctx.seenInBranch(ssVars), "+=", "=")) + condStr(b.hasSeen(ssVars), "+=", "=")) fix := mkline.Autofix() fix.Notef("The substitution command %q can be replaced with \"%s %s\".", @@ -291,13 +495,43 @@ func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) { // assignment operators on them. It's probably not worth the // effort, though. - ctx.seen().set(ssVars | ssVarsAutofix) + b.addSeen(ssVars) + b.addSeen(ssVarsAutofix) + } +} + +func (b *substBlock) dupString(mkline *MkLine, part substSeen) { + if b.hasSeen(part) { + mkline.Warnf("Duplicate definition of %q.", mkline.Varname()) + } + b.addSeen(part) +} + +func (b *substBlock) dupList(mkline *MkLine, part substSeen, autofixPart substSeen) { + if b.hasSeenAnywhere(part) && mkline.Op() != opAssignAppend { + b.fixOperatorAppend(mkline, b.hasSeen(autofixPart)) + } + b.addSeen(part) +} + +func (b *substBlock) fixOperatorAppend(mkline *MkLine, dueToAutofix bool) { + before := mkline.ValueAlign() + after := alignWith(mkline.Varname()+"+=", before) + + fix := mkline.Autofix() + if dueToAutofix { + fix.Notef(SilentAutofixFormat) + } else { + fix.Warnf("All but the first assignment to %q should use the \"+=\" operator.", + mkline.Varname()) } + fix.Replace(before, after) + fix.Apply() } // extractVarname extracts the variable name from a sed command of the form // s,@VARNAME@,${VARNAME}, and some related variants thereof. -func (*SubstContext) extractVarname(token string) string { +func (*substBlock) extractVarname(token string) string { parser := NewMkLexer(token, nil) lexer := parser.lexer if !lexer.SkipByte('s') { @@ -337,235 +571,111 @@ func (*SubstContext) extractVarname(token string) string { return varname } -func (*SubstContext) isForeign(varcanon string) bool { - switch varcanon { - case - "SUBST_STAGE.*", - "SUBST_MESSAGE.*", - "SUBST_FILES.*", - "SUBST_SED.*", - "SUBST_VARS.*", - "SUBST_FILTER_CMD.*": - return false - } - return true +func (b *substBlock) isComplete() bool { + return b.allSeen().hasAll(ssStage | ssFiles | ssTransform) } -func (*SubstContext) isListCanon(varcanon string) bool { - switch varcanon { - case - "SUBST_FILES.*", - "SUBST_SED.*", - "SUBST_VARS.*": - return true +func (b *substBlock) hasSeen(part substSeen) bool { + for _, cond := range b.conds { + if cond.hasSeen(part) { + return true + } } return false } -func (ctx *SubstContext) directive(mkline *MkLine) { - dir := mkline.Directive() - switch dir { - case "if": - ctx.condIf() - - case "elif", "else": - ctx.condElse(mkline, dir) - - case "endif": - ctx.condEndif(mkline) +func (b *substBlock) hasSeenAnywhere(part substSeen) bool { + for _, cond := range b.conds { + if cond.hasSeenAnywhere(part) { + return true + } } + return false } -func (ctx *SubstContext) condIf() { - top := substCond{total: ssAll} - ctx.conds = append(ctx.conds, &top) +func (b *substBlock) allSeen() substSeen { + all := ssNone + for _, cond := range b.conds { + all.addAll(cond.curr) + } + return all } -func (ctx *SubstContext) condElse(mkline *MkLine, dir string) { - top := ctx.cond() - top.total.retain(top.curr) - if !ctx.isConditional() { - ctx.finishClass(mkline) - } - top.curr = ssNone - top.seenElse = dir == "else" +func (b *substBlock) addSeen(part substSeen) { + b.conds[len(b.conds)-1].addSeen(part) } -func (ctx *SubstContext) condEndif(diag Diagnoser) { - top := ctx.cond() - top.total.retain(top.curr) - if !ctx.isConditional() { - ctx.finishClass(diag) - } - if !top.seenElse { - top.total = ssNone - } - if len(ctx.conds) > 1 { - ctx.conds = ctx.conds[:len(ctx.conds)-1] - } - ctx.seen().union(top.total) +func (b *substBlock) rememberForeign(mkline *MkLine) { + b.foreign = append(b.foreign, mkline) } -func (ctx *SubstContext) finishClass(diag Diagnoser) { - if !ctx.isActive() { - return - } +// isConditional returns whether the current line is at a deeper conditional +// level than the corresponding SUBST_CLASSES line. +func (b *substBlock) isConditional() bool { return len(b.conds) > 1 } - if ctx.seen().get(ssAll) { - ctx.checkBlockComplete(diag) - ctx.checkForeignVariables() - } else { - ctx.markAsNotDone() +func (b *substBlock) allowVar(varname string) { + if b.foreignAllowed == nil { + b.foreignAllowed = map[string]struct{}{} } - - ctx.reset() + b.foreignAllowed[varname] = struct{}{} } -func (ctx *SubstContext) checkBlockComplete(diag Diagnoser) { - id := ctx.activeId() - seen := ctx.seen() - if !seen.get(ssStage) { - diag.Warnf("Incomplete SUBST block: SUBST_STAGE.%s missing.", id) - } - if !seen.get(ssFiles) { - diag.Warnf("Incomplete SUBST block: SUBST_FILES.%s missing.", id) - } - if !seen.get(ssTransform) { - diag.Warnf("Incomplete SUBST block: SUBST_SED.%[1]s, SUBST_VARS.%[1]s or SUBST_FILTER_CMD.%[1]s missing.", id) - } -} +func (b *substBlock) enter() { b.conds = append(b.conds, newSubstCond()) } -func (ctx *SubstContext) checkForeignVariables() { - ctx.forEachForeignVar(func(mkline *MkLine) { - mkline.Warnf("Foreign variable %q in SUBST block.", mkline.Varname()) - }) +func (b *substBlock) nextBranch(isElse bool) { + cond := b.conds[len(b.conds)-1] + cond.leaveBranch() + cond.enterBranch(isElse) } -func (ctx *SubstContext) finishFile(diag Diagnoser) { - for _, id := range ctx.queuedIds { - if id != "" && !ctx.isDone(id) { - ctx.warnUndefinedBlock(diag, id) - } - } -} +func (b *substBlock) leave() { + assert(b.isConditional()) -func (*SubstContext) warnUndefinedBlock(diag Diagnoser, id string) { - diag.Warnf("Missing SUBST block for %q.", id) - diag.Explain( - "After adding a SUBST class to SUBST_CLASSES,", - "the remaining SUBST variables should be defined in the same file.", - "", - "See mk/subst.mk for the comprehensive documentation.") + n := len(b.conds) + b.conds[n-2].leaveLevel(b.conds[n-1]) + b.conds = b.conds[:n-1] } -// In the paragraph of a SUBST block, there should be only variables -// that actually belong to the SUBST block. -// -// In addition, variables that are mentioned in SUBST_VARS may also -// be defined there because they closely relate to the SUBST block. +func (b *substBlock) finish(diag Diagnoser) { + assert(len(b.conds) == 1) -func (ctx *SubstContext) allowVar(varname string) { - if ctx.foreignAllowed == nil { - ctx.foreignAllowed = make(map[string]struct{}) + if b.done { + return } - ctx.foreignAllowed[varname] = struct{}{} -} + b.done = true -func (ctx *SubstContext) rememberForeign(mkline *MkLine) { - ctx.foreign = append(ctx.foreign, mkline) -} - -// forEachForeignVar performs the given action for each variable that -// is defined in the SUBST block and is not mentioned in SUBST_VARS. -func (ctx *SubstContext) forEachForeignVar(action func(*MkLine)) { - for _, foreign := range ctx.foreign { - if _, ok := ctx.foreignAllowed[foreign.Varname()]; !ok { - action(foreign) - } + if !b.hasStarted() { + diag.Warnf("Missing SUBST block for %q.", b.id) + return } -} - -func (ctx *SubstContext) reset() { - ctx.id = "" - ctx.foreignAllowed = nil - ctx.foreign = nil - ctx.conds = []*substCond{{seenElse: true}} -} - -func (ctx *SubstContext) isActive() bool { return ctx.id != "" } - -func (ctx *SubstContext) isActiveId(id string) bool { return ctx.id == id } - -func (ctx *SubstContext) activeId() string { - assert(ctx.isActive()) - return ctx.id -} - -func (ctx *SubstContext) setActiveId(id string) { - ctx.id = id - ctx.cond().top = true - ctx.markAsDone(id) -} -func (ctx *SubstContext) queue(id string) { - ctx.queuedIds = append(ctx.queuedIds, id) -} - -func (ctx *SubstContext) start(id string) bool { - for i, queuedId := range ctx.queuedIds { - if queuedId == id { - ctx.queuedIds[i] = "" - ctx.setActiveId(id) - return true - } + if !b.hasSeen(ssStage) { + diag.Warnf("Incomplete SUBST block: SUBST_STAGE.%s missing.", b.id) } - return false -} - -func (ctx *SubstContext) markAsDone(id string) { - if ctx.doneIds == nil { - ctx.doneIds = map[string]bool{} + if !b.hasSeen(ssFiles) { + diag.Warnf("Incomplete SUBST block: SUBST_FILES.%s missing.", b.id) + } + if !b.hasSeen(ssTransform) { + diag.Warnf( + "Incomplete SUBST block: SUBST_SED.%[1]s, "+ + "SUBST_VARS.%[1]s or SUBST_FILTER_CMD.%[1]s missing.", + b.id) } - ctx.doneIds[id] = true -} - -func (ctx *SubstContext) markAsNotDone() { - ctx.doneIds[ctx.id] = false -} - -func (ctx *SubstContext) isDone(varparam string) bool { - return ctx.doneIds[varparam] -} - -func (ctx *SubstContext) isComplete() bool { - return ctx.seen().hasAll(ssStage | ssFiles | ssTransform) -} - -// isConditional returns whether the current line is at a deeper conditional -// level than the assignment where the corresponding class ID is added to -// SUBST_CLASSES. -// -// TODO: Adjust the implementation to this description. -func (ctx *SubstContext) isConditional() bool { - return !ctx.cond().top -} -// cond returns information about the parts of the SUBST block that -// have already been seen in the current leaf branch of the conditionals. -func (ctx *SubstContext) seen() *substSeen { - return &ctx.cond().curr + b.checkForeignVariables() } -// cond returns information about the current branch of conditionals. -func (ctx *SubstContext) cond() *substCond { - return ctx.conds[len(ctx.conds)-1] +func (b *substBlock) checkForeignVariables() { + for _, mkline := range b.foreign { + if _, ok := b.foreignAllowed[mkline.Varname()]; !ok { + mkline.Warnf("Foreign variable %q in SUBST block.", mkline.Varname()) + } + } } -// Returns true if the given flag from substSeen has been seen -// somewhere in the conditional path of the current line. -func (ctx *SubstContext) seenInBranch(part substSeen) bool { - for _, cond := range ctx.conds { - if cond.curr.get(part) { +func (b *substBlock) hasStarted() bool { + for _, cond := range b.conds { + if cond.currAny != ssNone { return true } } @@ -573,23 +683,16 @@ func (ctx *SubstContext) seenInBranch(part substSeen) bool { } type substCond struct { - // Tells whether a SUBST block has started at this conditional level. - // All variable assignments that belong to this class must happen at - // this conditional level or below it. - // - // TODO: For Test_SubstContext_Directive__conditional_complete, - // this needs to be changed to the set of classes that have been - // added to SUBST_CLASSES at this level. - top bool - // Collects the parts of the SUBST block that have been defined in all // branches that have been parsed completely. - total substSeen + total substSeen + totalAny substSeen // Collects the parts of the SUBST block that are defined in the current // branch of the conditional. At the end of the branch, they are merged // into the total. - curr substSeen + curr substSeen + currAny substSeen // Marks whether the current conditional statement has // an .else branch. If it doesn't, this means that all variables @@ -597,6 +700,40 @@ type substCond struct { seenElse bool } +func newSubstCond() *substCond { return &substCond{total: ssAll} } + +func (c *substCond) enterBranch(isElse bool) { + c.curr = ssNone + c.currAny = ssNone + c.seenElse = isElse +} + +func (c *substCond) leaveBranch() { + c.total.retainAll(c.curr) + c.totalAny.addAll(c.currAny) + c.curr = ssNone + c.currAny = ssNone +} + +func (c *substCond) leaveLevel(child *substCond) { + child.leaveBranch() + if child.seenElse { + c.curr.addAll(child.total) + } + c.currAny.addAll(child.totalAny) +} + +func (c *substCond) hasSeen(part substSeen) bool { return c.curr.has(part) } + +func (c *substCond) hasSeenAnywhere(part substSeen) bool { + return c.currAny.has(part) +} + +func (c *substCond) addSeen(part substSeen) { + c.curr.set(part) + c.currAny.set(part) +} + // substSeen contains all variables that depend on a particular SUBST // class ID. These variables can be set in conditional branches, and // pkglint keeps track whether they are set in all branches or only @@ -617,8 +754,16 @@ const ( ssNone substSeen = 0 ) -func (s *substSeen) set(part substSeen) { *s |= part } -func (s *substSeen) get(part substSeen) bool { return *s&part != 0 } -func (s *substSeen) hasAll(other substSeen) bool { return *s&other == other } -func (s *substSeen) union(other substSeen) { *s |= other } -func (s *substSeen) retain(other substSeen) { *s &= other } +func (s *substSeen) set(part substSeen) { + assert(part&(part-1) == 0) + *s |= part +} + +func (s *substSeen) has(part substSeen) bool { + assert(part&(part-1) == 0) + return *s&part != 0 +} + +func (s substSeen) hasAll(other substSeen) bool { return s&other == other } +func (s *substSeen) addAll(other substSeen) { *s |= other } +func (s *substSeen) retainAll(other substSeen) { *s &= other } diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go index f8bd4b67e99..8cae3f3c451 100644 --- a/pkgtools/pkglint/files/substcontext_test.go +++ b/pkgtools/pkglint/files/substcontext_test.go @@ -5,6 +5,7 @@ import "gopkg.in/check.v1" func (t *Tester) NewSubstAutofixTest(lines ...string) func(bool) { return func(autofix bool) { mklines := t.NewMkLines("filename.mk", lines...) + mklines.collectRationale() ctx := NewSubstContext() mklines.ForEach(ctx.Process) @@ -18,111 +19,13 @@ func (t *Tester) RunSubst(lines ...string) { assert(lines[len(lines)-1] != "") mklines := t.NewMkLines("filename.mk", lines...) + mklines.collectRationale() ctx := NewSubstContext() mklines.ForEach(ctx.Process) ctx.Finish(mklines.EOFLine()) } -func (s *Suite) Test_SubstContext__OPSYSVARS(c *check.C) { - t := s.Init(c) - - ctx := NewSubstContext() - - // SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk. - ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES.SunOS+=prefix")) - ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_CLASSES.NetBSD+=prefix")) - ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_FILES.prefix=Makefile")) - ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g")) - ctx.varassign(t.NewMkLine("filename.mk", 15, "SUBST_STAGE.prefix=post-configure")) - - t.CheckEquals(ctx.isComplete(), true) - - ctx.Finish(t.NewMkLine("filename.mk", 15, "")) - - t.CheckOutputLines( - "NOTE: filename.mk:14: The substitution command \"s,@PREFIX@,${PREFIX},g\" " + - "can be replaced with \"SUBST_VARS.prefix= PREFIX\".") -} - -func (s *Suite) Test_SubstContext__no_class(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "UNRELATED=anything", - "SUBST_FILES.repl+=Makefile.in", - "SUBST_SED.repl+=-e s,from,to,g") - - t.CheckOutputLines( - "WARN: filename.mk:2: Before defining SUBST_FILES.repl, " + - "the SUBST class should be declared using \"SUBST_CLASSES+= repl\".") -} - -func (s *Suite) Test_SubstContext__multiple_classes_in_one_line(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= one two", - "SUBST_STAGE.one= post-configure", - "SUBST_FILES.one= one.txt", - "SUBST_SED.one= s,one,1,g", - "SUBST_STAGE.two= post-configure", - "SUBST_FILES.two= two.txt") - - t.CheckOutputLines( - "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", - "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.") -} - -func (s *Suite) Test_SubstContext__multiple_classes_in_one_line_multiple_blocks(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= one two", - "SUBST_STAGE.one= post-configure", - "SUBST_FILES.one= one.txt", - "SUBST_SED.one= s,one,1,g", - "", - "SUBST_STAGE.two= post-configure", - "SUBST_FILES.two= two.txt", - "", - "SUBST_STAGE.three= post-configure", - "", - "SUBST_VARS.four= PREFIX", - "", - "SUBST_VARS.three= PREFIX") - - t.CheckOutputLines( - "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", - "WARN: filename.mk:8: Incomplete SUBST block: "+ - "SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.", - "WARN: filename.mk:9: Before defining SUBST_STAGE.three, "+ - "the SUBST class should be declared using \"SUBST_CLASSES+= three\".", - "WARN: filename.mk:11: Before defining SUBST_VARS.four, "+ - "the SUBST class should be declared using \"SUBST_CLASSES+= four\".") -} - -func (s *Suite) Test_SubstContext__multiple_classes_in_one_block(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= one", - "SUBST_STAGE.one= post-configure", - "SUBST_STAGE.one= post-configure", - "SUBST_FILES.one= one.txt", - "SUBST_CLASSES+= two", // The block "one" is not finished yet. - "SUBST_SED.one= s,one,1,g", - "SUBST_STAGE.two= post-configure", - "SUBST_FILES.two= two.txt", - "SUBST_SED.two= s,two,2,g") - - t.CheckOutputLines( - "WARN: filename.mk:3: Duplicate definition of \"SUBST_STAGE.one\".", - "WARN: filename.mk:5: Incomplete SUBST block: SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.", - "WARN: filename.mk:5: Subst block \"one\" should be finished before adding the next class to SUBST_CLASSES.", - "WARN: filename.mk:6: Variable \"SUBST_SED.one\" does not match SUBST class \"two\".") -} - // This is a strange example that probably won't occur in practice. // // Continuing a SUBST class in one of the branches and starting @@ -147,26 +50,7 @@ func (s *Suite) Test_SubstContext__partially_continued_class_in_conditional(c *c t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext__files_missing(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= one", - "SUBST_STAGE.one= pre-configure", - "SUBST_CLASSES+= two", - "SUBST_STAGE.two= pre-configure", - "SUBST_FILES.two= two.txt", - "SUBST_SED.two= s,two,2,g") - - t.CheckOutputLines( - "WARN: filename.mk:3: Incomplete SUBST block: SUBST_FILES.one missing.", - "WARN: filename.mk:3: Incomplete SUBST block: "+ - "SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.", - "WARN: filename.mk:3: Subst block \"one\" should be finished "+ - "before adding the next class to SUBST_CLASSES.") -} - -func (s *Suite) Test_SubstContext__directives(c *check.C) { +func (s *Suite) Test_SubstContext__conditionals(c *check.C) { t := s.Init(c) t.RunSubst( @@ -193,88 +77,75 @@ func (s *Suite) Test_SubstContext__directives(c *check.C) { "to \"SUBST_SED.os\" should use the \"+=\" operator.") } -func (s *Suite) Test_SubstContext__adjacent(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+=\t1", - "SUBST_STAGE.1=\tpre-configure", - "SUBST_FILES.1=\tfile1", - "SUBST_SED.1=\t-e s,subst1,repl1,", - "SUBST_CLASSES+=\t2", - "SUBST_SED.1+=\t-e s,subst1b,repl1b,", // Misplaced - "SUBST_STAGE.2=\tpre-configure", - "SUBST_FILES.2=\tfile2", - "SUBST_SED.2=\t-e s,subst2,repl2,") - - t.CheckOutputLines( - "WARN: filename.mk:6: Variable \"SUBST_SED.1\" does not match SUBST class \"2\".") -} - -func (s *Suite) Test_SubstContext__do_patch(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+=\tos", - "SUBST_STAGE.os=\tdo-patch", - "SUBST_FILES.os=\tguess-os.h", - "SUBST_SED.os=\t-e s,@OPSYS@,Darwin,") - - // No warning, since there is nothing to fix automatically. - // This case doesn't occur in practice anyway. - t.CheckOutputEmpty() -} - -// Variables mentioned in SUBST_VARS are not considered "foreign" -// in the block and may be mixed with the other SUBST variables. -func (s *Suite) Test_SubstContext__SUBST_VARS_defined_in_block(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__no_class(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+=\tos", - "SUBST_STAGE.os=\tpre-configure", - "SUBST_FILES.os=\tguess-os.h", - "SUBST_VARS.os=\tTODAY1", - "TODAY1!=\tdate", - "TODAY2!=\tdate") + "UNRELATED=anything", + "SUBST_FILES.repl+=Makefile.in", + "SUBST_SED.repl+=-e s,from,to,g") t.CheckOutputLines( - "WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.") + "WARN: filename.mk:2: Before defining SUBST_FILES.repl, " + + "the SUBST class should be declared using \"SUBST_CLASSES+= repl\".") } -// Variables mentioned in SUBST_VARS may appear in the same paragraph, -// or alternatively anywhere else in the file. -func (s *Suite) Test_SubstContext__SUBST_VARS_in_next_paragraph(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__multiple_classes_in_one_line_multiple_blocks(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+=\tos", - "SUBST_STAGE.os=\tpre-configure", - "SUBST_FILES.os=\tguess-os.h", - "SUBST_VARS.os=\tTODAY1", + "SUBST_CLASSES+= one two", + "SUBST_STAGE.one= post-configure", + "SUBST_FILES.one= one.txt", + "SUBST_SED.one= s,one,1,g", "", - "TODAY1!=\tdate", - "TODAY2!=\tdate") + "SUBST_STAGE.two= post-configure", + "SUBST_FILES.two= two.txt", + "", + "SUBST_STAGE.three= post-configure", + "", + "SUBST_VARS.four= PREFIX", + "", + "SUBST_VARS.three= PREFIX") - t.CheckOutputEmpty() + t.CheckOutputLines( + "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", + "WARN: filename.mk:9: Variable \"SUBST_STAGE.three\" "+ + "does not match SUBST class \"two\".", + "WARN: filename.mk:9: Incomplete SUBST block: "+ + "SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.", + "WARN: filename.mk:9: Before defining SUBST_STAGE.three, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= three\".", + "WARN: filename.mk:11: Before defining SUBST_VARS.four, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= four\".") } -func (s *Suite) Test_SubstContext__multiple_SUBST_VARS(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__multiple_classes_in_one_block(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+=\tos", - "SUBST_STAGE.os=\tpre-configure", - "SUBST_FILES.os=\tguess-os.h", - "SUBST_VARS.os=\tPREFIX VARBASE") + "SUBST_CLASSES+= one", + "SUBST_STAGE.one= post-configure", + "SUBST_STAGE.one= post-configure", + "SUBST_FILES.one= one.txt", + "SUBST_CLASSES+= two", // The block "one" is not finished yet. + "SUBST_SED.one= s,one,1,g", + "SUBST_STAGE.two= post-configure", + "SUBST_FILES.two= two.txt", + "SUBST_SED.two= s,two,2,g") - t.CheckOutputEmpty() + t.CheckOutputLines( + "WARN: filename.mk:3: Duplicate definition of \"SUBST_STAGE.one\".", + "WARN: filename.mk:5: Subst block \"one\" should be finished "+ + "before adding the next class to SUBST_CLASSES.", + "WARN: filename.mk:5: Incomplete SUBST block: SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.", + "WARN: filename.mk:6: Late additions to a SUBST variable should use the += operator.") } -// As of May 2019, pkglint does not check the order of the variables in +// As of December 2019, pkglint does not check the order of the variables in // a SUBST block. Enforcing this order, or at least suggesting it, would // make pkgsrc packages more uniform, which is a good idea, but not urgent. -func (s *Suite) Test_SubstContext__unusual_variable_order(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__unusual_order(c *check.C) { t := s.Init(c) t.RunSubst( @@ -287,41 +158,7 @@ func (s *Suite) Test_SubstContext__unusual_variable_order(c *check.C) { t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext__completely_conditional_then(c *check.C) { - t := s.Init(c) - - t.RunSubst( - ".if ${OPSYS} == Linux", - "SUBST_CLASSES+=\tid", - "SUBST_STAGE.id=\tpre-configure", - "SUBST_SED.id=\t-e sahara", - ".else", - ".endif") - - // The block already ends at the .else, not at the end of the file, - // since that is the scope where the SUBST id is defined. - t.CheckOutputLines( - "WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.id missing.") -} - -func (s *Suite) Test_SubstContext__completely_conditional_else(c *check.C) { - t := s.Init(c) - - t.RunSubst( - ".if ${OPSYS} == Linux", - ".else", - "SUBST_CLASSES+=\tid", - "SUBST_STAGE.id=\tpre-configure", - "SUBST_SED.id=\t-e sahara", - ".endif") - - // The block already ends at the .endif, not at the end of the file, - // since that is the scope where the SUBST id is defined. - t.CheckOutputLines( - "WARN: filename.mk:6: Incomplete SUBST block: SUBST_FILES.id missing.") -} - -func (s *Suite) Test_SubstContext__SUBST_CLASSES_in_separate_paragraph(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__blocks_in_separate_paragraphs(c *check.C) { t := s.Init(c) t.RunSubst( @@ -344,7 +181,7 @@ func (s *Suite) Test_SubstContext__SUBST_CLASSES_in_separate_paragraph(c *check. "WARN: filename.mk:EOF: Missing SUBST block for \"4\".") } -func (s *Suite) Test_SubstContext__wrong_class(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__typo_in_id(c *check.C) { t := s.Init(c) t.RunSubst( @@ -358,33 +195,9 @@ func (s *Suite) Test_SubstContext__wrong_class(c *check.C) { t.CheckOutputLines( "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", - "WARN: filename.mk:2: Variable \"SUBST_STAGE.x\" does not match SUBST class \"1\".", - "WARN: filename.mk:3: Variable \"SUBST_FILES.x\" does not match SUBST class \"1\".", - "WARN: filename.mk:4: Variable \"SUBST_VARS.x\" does not match SUBST class \"1\".", - // XXX: This line could change to 2, since that is already in the queue. - "WARN: filename.mk:5: Variable \"SUBST_STAGE.2\" does not match SUBST class \"1\".", - "WARN: filename.mk:6: Variable \"SUBST_FILES.2\" does not match SUBST class \"1\".", - "WARN: filename.mk:7: Variable \"SUBST_VARS.2\" does not match SUBST class \"1\".", - "WARN: filename.mk:EOF: Missing SUBST block for \"1\".", - "WARN: filename.mk:EOF: Missing SUBST block for \"2\".") -} - -func (s *Suite) Test_SubstContext_varassign__late_addition(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+=\tid", - "SUBST_STAGE.id=\tpost-configure", - "SUBST_FILES.id=\tfiles", - "SUBST_VARS.id=\tPREFIX", - "", - ".if ${OPSYS} == NetBSD", - "SUBST_VARS.id=\tOPSYS", - ".endif") - - t.CheckOutputLines( - "WARN: filename.mk:7: Late additions to a SUBST variable " + - "should use the += operator.") + "WARN: filename.mk:2: Before defining SUBST_STAGE.x, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= x\".", + "WARN: filename.mk:EOF: Missing SUBST block for \"1\".") } func (s *Suite) Test_SubstContext_varassign__late_addition_to_unknown_class(c *check.C) { @@ -403,31 +216,156 @@ func (s *Suite) Test_SubstContext_varassign__late_addition_to_unknown_class(c *c "the SUBST class should be declared using \"SUBST_CLASSES+= id\".") } -func (s *Suite) Test_SubstContext_varassignClasses__none(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__rationale(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+=\t# none") - + "# Adjust setup.py", + "SUBST_CLASSES+= setup", + "SUBST_STAGE.setup= post-configure", + "SUBST_FILES.setup= setup.py", + "SUBST_VARS.setup= VAR") + + // The rationale in line 1 is supposed to suppress warnings, + // not add new ones. t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext_varassignClasses__indirect(c *check.C) { +func (s *Suite) Test_SubstContext_varassign__interleaved(c *check.C) { t := s.Init(c) - t.SetUpVartypes() - mklines := t.NewMkLines("filename.mk", - MkCvsID, - "SUBST_CLASSES+=\t${VAR}") - - mklines.Check() - - t.CheckOutputLines( + t.RunSubst( + "SUBST_CLASSES+= 1 2 3", + "SUBST_STAGE.1= post-configure", + "SUBST_STAGE.2= post-configure", + "SUBST_STAGE.3= post-configure", + "SUBST_FILES.1= setup.py", + "SUBST_FILES.2= setup.py", + "SUBST_FILES.3= setup.py", + "SUBST_VARS.1= VAR", + "SUBST_VARS.2= VAR", + "SUBST_VARS.3= VAR") + + // The above does not follow the common pattern of defining + // each block on its own. + // It technically works but is not easy to read for humans. + t.CheckOutputLines( + "NOTE: filename.mk:1: " + + "Please add only one class at a time to SUBST_CLASSES.") +} + +func (s *Suite) Test_SubstContext_varassignClasses__OPSYSVARS(c *check.C) { + t := s.Init(c) + + ctx := NewSubstContext() + + // SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk. + ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES.SunOS+=prefix")) + ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_CLASSES.NetBSD+=prefix")) + ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_FILES.prefix=Makefile")) + ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g")) + ctx.varassign(t.NewMkLine("filename.mk", 15, "SUBST_STAGE.prefix=post-configure")) + + t.CheckEquals(ctx.block().isComplete(), true) + + ctx.Finish(t.NewMkLine("filename.mk", 15, "")) + + t.CheckOutputLines( + "NOTE: filename.mk:14: The substitution command \"s,@PREFIX@,${PREFIX},g\" " + + "can be replaced with \"SUBST_VARS.prefix= PREFIX\".") +} + +func (s *Suite) Test_SubstContext_varassignClasses__duplicate_id(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= id", + "SUBST_CLASSES+= id") + + t.CheckOutputLines( + "ERROR: filename.mk:2: Duplicate SUBST class \"id\".", + "WARN: filename.mk:EOF: Missing SUBST block for \"id\".") +} + +func (s *Suite) Test_SubstContext_varassignClasses__multiple_classes_in_one_line(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= one two", + "SUBST_STAGE.one= post-configure", + "SUBST_FILES.one= one.txt", + "SUBST_SED.one= s,one,1,g", + "SUBST_STAGE.two= post-configure", + "SUBST_FILES.two= two.txt") + + t.CheckOutputLines( + "NOTE: filename.mk:1: Please add only one class at a time to SUBST_CLASSES.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.") +} + +func (s *Suite) Test_SubstContext_varassignClasses__none(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\t# none") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_SubstContext_varassignClasses__indirect(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("filename.mk", + MkCvsID, + "SUBST_CLASSES+=\t${VAR}") + + mklines.Check() + + t.CheckOutputLines( "ERROR: filename.mk:2: Identifiers for SUBST_CLASSES "+ "must not refer to other variables.", "WARN: filename.mk:2: VAR is used but not defined.") } +func (s *Suite) Test_SubstContext_varassignOutsideBlock__assign(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\t1", + "SUBST_STAGE.1=\tpre-configure", + "SUBST_FILES.1=\tfile1", + "SUBST_SED.1=\t-e s,subst1,repl1,", + "SUBST_CLASSES+=\t2", + "SUBST_SED.1=\t-e s,subst1b,repl1b,", // Misplaced + "SUBST_STAGE.2=\tpre-configure", + "SUBST_FILES.2=\tfile2", + "SUBST_SED.2=\t-e s,subst2,repl2,") + + t.CheckOutputLines( + "WARN: filename.mk:6: Late additions to a SUBST variable " + + "should use the += operator.") +} + +func (s *Suite) Test_SubstContext_varassignOutsideBlock__append(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\t1", + "SUBST_STAGE.1=\tpre-configure", + "SUBST_FILES.1=\tfile1", + "SUBST_SED.1=\t-e s,subst1,repl1,", + "SUBST_CLASSES+=\t2", + "SUBST_SED.1+=\t-e s,subst1b,repl1b,", // Misplaced + "SUBST_STAGE.2=\tpre-configure", + "SUBST_FILES.2=\tfile2", + "SUBST_SED.2=\t-e s,subst2,repl2,") + + // The SUBST_SED.1 line is misplaced. It uses the += operator, + // which makes it unclear whether this is a typo or not. + t.CheckOutputEmpty() +} + // The rationale for the stray SUBST variables has to be specific. // // For example, in the following snippet from mail/dkim-milter/options.mk @@ -460,19 +398,432 @@ func (s *Suite) Test_SubstContext_varassignOutsideBlock__rationale(c *check.C) { ctx := NewSubstContext() mklines.collectRationale() - mklines.ForEach(ctx.Process) + mklines.ForEach(ctx.Process) + + t.CheckOutputLines( + "WARN: filename.mk:2: Before defining SUBST_VARS.one, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= one\".", + // In filename.mk:5 there is a proper rationale, thus no warning. + "WARN: filename.mk:8: Before defining SUBST_VARS.def, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= def\".", + "WARN: filename.mk:11: Before defining SUBST_SED.libs, "+ + "the SUBST class should be declared using \"SUBST_CLASSES+= libs\".") +} + +// Unbalanced conditionals must not lead to a panic. +func (s *Suite) Test_SubstContext_directive__before_SUBST_CLASSES(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if 0", + ".endif", + "SUBST_CLASSES+=\tos", + ".elif 0") + + t.CheckOutputLines( + "WARN: filename.mk:4: Missing SUBST block for \"os\".") +} + +func (s *Suite) Test_SubstContext_directive__conditional_blocks_complete(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= nb", + "SUBST_STAGE.nb= post-configure", + "SUBST_FILES.nb= guess-netbsd.h", + "SUBST_VARS.nb= HAVE_NETBSD", + ".else", + "SUBST_CLASSES+= os", + "SUBST_STAGE.os= post-configure", + "SUBST_FILES.os= guess-netbsd.h", + "SUBST_VARS.os= HAVE_OTHER", + ".endif") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_SubstContext_directive__conditional_blocks_incomplete(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= nb", + "SUBST_STAGE.nb= post-configure", + "SUBST_VARS.nb= HAVE_NETBSD", + ".else", + "SUBST_CLASSES+= os", + "SUBST_STAGE.os= post-configure", + "SUBST_FILES.os= guess-netbsd.h", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.nb missing.", + "WARN: filename.mk:6: Subst block \"nb\" should be finished "+ + "before adding the next class to SUBST_CLASSES.", + "WARN: filename.mk:9: Incomplete SUBST block: "+ + "SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") +} + +func (s *Suite) Test_SubstContext_directive__conditional_complete(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= id", + ".if ${OPSYS} == NetBSD", + "SUBST_STAGE.id=\t\tpost-configure", + "SUBST_MESSAGE.id=\tpost-configure", + "SUBST_FILES.id=\t\tguess-netbsd.h", + "SUBST_SED.id=\t\t-e s,from,to,", + "SUBST_VARS.id=\t\tHAVE_OTHER", + "SUBST_FILTER_CMD.id=\tHAVE_OTHER", + ".else", + "SUBST_STAGE.id=\t\tpost-configure", + "SUBST_MESSAGE.id=\tpost-configure", + "SUBST_FILES.id=\t\tguess-netbsd.h", + "SUBST_SED.id=\t\t-e s,from,to,", + "SUBST_VARS.id=\t\tHAVE_OTHER", + "SUBST_FILTER_CMD.id=\tHAVE_OTHER", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:3: SUBST_STAGE.id should not be defined conditionally.", + "WARN: filename.mk:4: SUBST_MESSAGE.id should not be defined conditionally.", + "WARN: filename.mk:10: SUBST_STAGE.id should not be defined conditionally.", + "WARN: filename.mk:11: SUBST_MESSAGE.id should not be defined conditionally.") +} + +func (s *Suite) Test_SubstContext_directive__conditionally_overwritten_filter(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= id", + "SUBST_STAGE.id=\t\tpost-configure", + "SUBST_MESSAGE.id=\tpost-configure", + "SUBST_FILES.id=\t\tguess-netbsd.h", + "SUBST_FILTER_CMD.id=\tHAVE_OTHER", + ".if ${OPSYS} == NetBSD", + "SUBST_FILTER_CMD.id=\tHAVE_OTHER", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:7: Duplicate definition of \"SUBST_FILTER_CMD.id\".") +} + +// Hopefully nobody will ever trigger this case in real pkgsrc. +// It's plain confusing to a casual reader to nest a complete +// SUBST block into another SUBST block. +func (s *Suite) Test_SubstContext_directive__conditionally_nested_block(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= outer", + "SUBST_STAGE.outer= post-configure", + "SUBST_FILES.outer= outer.txt", + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= inner", + "SUBST_STAGE.inner= post-configure", + "SUBST_FILES.inner= inner.txt", + "SUBST_VARS.inner= INNER", + ".endif", + "SUBST_VARS.outer= OUTER") + + t.CheckOutputLines( + "WARN: filename.mk:5: Subst block \"outer\" should be finished " + + "before adding the next class to SUBST_CLASSES.") +} + +// It's completely valid to have several SUBST blocks in a single paragraph. +// As soon as a SUBST_CLASSES line appears, pkglint assumes that all previous +// SUBST blocks are finished. That's exactly the case here. +func (s *Suite) Test_SubstContext_directive__conditionally_following_block(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= outer", + "SUBST_STAGE.outer= post-configure", + "SUBST_FILES.outer= outer.txt", + "SUBST_VARS.outer= OUTER", + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= middle", + "SUBST_STAGE.middle= post-configure", + "SUBST_FILES.middle= inner.txt", + "SUBST_VARS.middle= INNER", + ". if ${MACHINE_ARCH} == amd64", + "SUBST_CLASSES+= inner", + "SUBST_STAGE.inner= post-configure", + "SUBST_FILES.inner= inner.txt", + "SUBST_VARS.inner= INNER", + ". endif", + ".endif") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_SubstContext_directive__two_blocks_in_condition(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= a", + "SUBST_STAGE.a= post-configure", + "SUBST_FILES.a= outer.txt", + "SUBST_VARS.a= OUTER", + "SUBST_CLASSES+= b", + "SUBST_STAGE.b= post-configure", + "SUBST_FILES.b= inner.txt", + "SUBST_VARS.b= INNER", + ".endif") + + // Up to 2019-12-12, pkglint wrongly warned in filename.mk:6: + // Subst block "a" should be finished before adding + // the next class to SUBST_CLASSES. + // The warning was wrong since block "a" has all required fields set. + // The warning was caused by an inconsistent check whether the current + // block had any conditional variables. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_SubstContext_directive__nested_conditional_incomplete_block(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= outer", + "SUBST_STAGE.outer= post-configure", + "SUBST_FILES.outer= outer.txt", + "SUBST_VARS.outer= OUTER", + ".if ${OPSYS} == NetBSD", + "SUBST_CLASSES+= inner1", + "SUBST_STAGE.inner1= post-configure", + "SUBST_VARS.inner1= INNER", + "SUBST_CLASSES+= inner2", + "SUBST_STAGE.inner2= post-configure", + "SUBST_FILES.inner2= inner.txt", + "SUBST_VARS.inner2= INNER", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:9: Subst block \"inner1\" should be finished "+ + "before adding the next class to SUBST_CLASSES.", + "WARN: filename.mk:9: Incomplete SUBST block: SUBST_FILES.inner1 missing.") +} + +func (s *Suite) Test_SubstContext_leave__details_in_then_branch(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= os", + ".if ${OPSYS} == NetBSD", + "SUBST_VARS.os= OPSYS", + "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", + "SUBST_STAGE.os= post-configure", + "SUBST_MESSAGE.os= Guessing operating system", + "SUBST_FILES.os= guess-os.h", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:5: SUBST_STAGE.os should not be defined conditionally.", + "WARN: filename.mk:6: SUBST_MESSAGE.os should not be defined conditionally.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_STAGE.os missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") +} + +func (s *Suite) Test_SubstContext_leave__details_in_else_branch(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= os", + ".if ${OPSYS} == NetBSD", + ".else", + "SUBST_VARS.os= OPSYS", + "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", + "SUBST_STAGE.os= post-configure", + "SUBST_MESSAGE.os= Guessing operating system", + "SUBST_FILES.os= guess-os.h", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:6: SUBST_STAGE.os should not be defined conditionally.", + "WARN: filename.mk:7: SUBST_MESSAGE.os should not be defined conditionally.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_STAGE.os missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") +} + +func (s *Suite) Test_SubstContext_leave__empty_conditional_at_end(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= os", + "SUBST_VARS.os= OPSYS", + "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", + "SUBST_STAGE.os= post-configure", + "SUBST_MESSAGE.os= Guessing operating system", + "SUBST_FILES.os= guess-os.h", + ".if ${OPSYS} == NetBSD", + ".else", + ".endif") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_SubstContext_leave__missing_transformation_in_one_branch(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= os", + "SUBST_STAGE.os= post-configure", + "SUBST_MESSAGE.os= Guessing operating system", + "SUBST_FILES.os= guess-os.h", + ".if ${OPSYS} == NetBSD", + "SUBST_FILES.os= -e s,@OpSYS@,NetBSD,", // A simple typo, this should be SUBST_SED. + ".elif ${OPSYS} == Darwin", + "SUBST_SED.os= -e s,@OPSYS@,Darwin1,", + "SUBST_SED.os= -e s,@OPSYS@,Darwin2,", + ".else", + "SUBST_VARS.os= OPSYS", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:6: All but the first assignment "+ + "to \"SUBST_FILES.os\" should use the \"+=\" operator.", + "WARN: filename.mk:9: All but the first assignment "+ + "to \"SUBST_SED.os\" should use the \"+=\" operator.", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.os, "+ + "SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") +} + +func (s *Suite) Test_SubstContext_leave__nested_conditionals(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= os", + "SUBST_STAGE.os= post-configure", + "SUBST_MESSAGE.os= Guessing operating system", + ".if ${OPSYS} == NetBSD", + "SUBST_FILES.os= guess-netbsd.h", + ". if ${ARCH} == i386", + "SUBST_FILTER_CMD.os= ${SED} -e s,@OPSYS,NetBSD-i386,", + ". elif ${ARCH} == x86_64", + "SUBST_VARS.os= OPSYS", + ". else", + "SUBST_SED.os= -e s,@OPSYS,NetBSD-unknown", + ". endif", + ".else", + // This branch omits SUBST_FILES. + "SUBST_SED.os= -e s,@OPSYS@,unknown,", + ".endif") + + t.CheckOutputLines( + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.") +} + +// With every .if directive, a new scope is created, to properly +// keep track of the conditional level at which the SUBST classes +// are declared. +// +// The scopes are even created when there is no SUBST variable +// anywhere close. The conditionals must be tracked for determining +// the end of the scope for the SUBST_CLASSES IDs. +func (s *Suite) Test_substScope__conditionals(c *check.C) { + t := s.Init(c) + + ctx := NewSubstContext() + + line := func(text string) { + mkline := t.NewMkLine("filename.mk", 123, text) + ctx.Process(mkline) + } + verifyScopes := func(n int) { + t.CheckEquals(len(ctx.scopes), n) + } + + verifyScopes(1) + + line(".if 1") + verifyScopes(2) + + line(". if 1") + verifyScopes(3) + + line(". if 1") + verifyScopes(4) + + line(". elif 1") + verifyScopes(4) + + line(". else") + verifyScopes(4) + + line(". endif") + verifyScopes(3) + + line(". endif") + verifyScopes(2) + + line(".endif") + verifyScopes(1) + + // An unbalanced .endif must not lead to a panic. + line(".endif") + verifyScopes(1) + + ctx.Finish(NewLineEOF("filename.mk")) +} + +func (s *Suite) Test_substScope_prepareSubstClasses(c *check.C) { + t := s.Init(c) + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= post-configure", + ".if 0") + + // There's no need to warn about unbalanced conditionals + // since that is already done by MkLines.Check. t.CheckOutputLines( - "WARN: filename.mk:2: Before defining SUBST_VARS.one, "+ - "the SUBST class should be declared using \"SUBST_CLASSES+= one\".", - // In filename.mk:5 there is a proper rationale, thus no warning. - "WARN: filename.mk:8: Before defining SUBST_VARS.def, "+ - "the SUBST class should be declared using \"SUBST_CLASSES+= def\".", - "WARN: filename.mk:11: Before defining SUBST_SED.libs, "+ - "the SUBST class should be declared using \"SUBST_CLASSES+= libs\".") + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.1 missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.") +} + +func (s *Suite) Test_substScope_prepareSubstClasses__nested(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= post-configure", + ".if 0", + ".if 0", + "SUBST_CLASSES+= 2") + + t.CheckOutputLines( + "WARN: filename.mk:5: Subst block \"1\" should be finished "+ + "before adding the next class to SUBST_CLASSES.", + "WARN: filename.mk:EOF: Missing SUBST block for \"2\".", + "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.1 missing.", + "WARN: filename.mk:EOF: Incomplete SUBST block: "+ + "SUBST_SED.1, SUBST_VARS.1 or SUBST_FILTER_CMD.1 missing.") +} + +func (s *Suite) Test_substBlock_varassignStage__do_patch(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\tos", + "SUBST_STAGE.os=\tdo-patch", + "SUBST_FILES.os=\tguess-os.h", + "SUBST_SED.os=\t-e s,@OPSYS@,Darwin,") + + // No warning, since there is nothing to fix automatically. + // This case doesn't occur in practice anyway. + t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext_varassignStage__pre_patch(c *check.C) { +func (s *Suite) Test_substBlock_varassignStage__pre_patch(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -495,7 +846,7 @@ func (s *Suite) Test_SubstContext_varassignStage__pre_patch(c *check.C) { "SUBST_SED.os= -e s,@OPSYS@,Darwin,") } -func (s *Suite) Test_SubstContext_varassignStage__post_patch(c *check.C) { +func (s *Suite) Test_substBlock_varassignStage__post_patch(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -518,12 +869,31 @@ func (s *Suite) Test_SubstContext_varassignStage__post_patch(c *check.C) { "SUBST_SED.os= -e s,@OPSYS@,Darwin,") } +func (s *Suite) Test_substBlock_varassignStage__empty_class(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + + t.RunSubst( + "SUBST_CLASSES+= id", + "", + "SUBST_STAGE.= post-patch", + "SUBST_STAGE.id= post-configure", + "SUBST_FILES.id= files", + "SUBST_VARS.id= VAR", + "SUBST_VARS.= VAR") + + t.CheckOutputLines( + "ERROR: filename.mk:3: Invalid SUBST class \"\" in variable name.", + "ERROR: filename.mk:7: Invalid SUBST class \"\" in variable name.") +} + // As of December 2019, pkglint does not use token positions internally. // Instead it only does simple string replacement when autofixing things. // To avoid damaging anything, replacements are only done if they are // unambiguous. This is not the case here, since line 4 contains the // string "pre-patch" twice. -func (s *Suite) Test_SubstContext_varassignStage__ambiguous_replacement(c *check.C) { +func (s *Suite) Test_substBlock_varassignStage__ambiguous_replacement(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -541,7 +911,7 @@ func (s *Suite) Test_SubstContext_varassignStage__ambiguous_replacement(c *check t.CheckEquals(t.File("filename.mk").IsFile(), false) } -func (s *Suite) Test_SubstContext_varassignStage__with_NO_CONFIGURE(c *check.C) { +func (s *Suite) Test_substBlock_varassignStage__with_NO_CONFIGURE(c *check.C) { t := s.Init(c) pkg := t.SetUpPackage("category/package", @@ -572,7 +942,7 @@ func (s *Suite) Test_SubstContext_varassignStage__with_NO_CONFIGURE(c *check.C) "when NO_CONFIGURE is set (in line 35).") } -func (s *Suite) Test_SubstContext_varassignStage__without_NO_CONFIGURE(c *check.C) { +func (s *Suite) Test_substBlock_varassignStage__without_NO_CONFIGURE(c *check.C) { t := s.Init(c) pkg := t.SetUpPackage("category/package", @@ -589,7 +959,7 @@ func (s *Suite) Test_SubstContext_varassignStage__without_NO_CONFIGURE(c *check. // Before 2019-12-12, pkglint wrongly warned about variables that were // not obviously SUBST variables, even if they were used later in SUBST_VARS. -func (s *Suite) Test_SubstContext_varassignVars__var_before_SUBST_VARS(c *check.C) { +func (s *Suite) Test_substBlock_varassignVars__var_before_SUBST_VARS(c *check.C) { t := s.Init(c) t.RunSubst( @@ -612,23 +982,19 @@ func (s *Suite) Test_SubstContext_varassignVars__var_before_SUBST_VARS(c *check. "WARN: filename.mk:4: Foreign variable \"FOREIGN\" in SUBST block.") } -func (s *Suite) Test_SubstContext_dupList__conditional_before_unconditional(c *check.C) { +func (s *Suite) Test_substBlock_varassignVars(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+= os", - "SUBST_STAGE.os= post-configure", - ".if 1", - "SUBST_FILES.os= conditional", - ".endif", - "SUBST_FILES.os= unconditional", - "SUBST_VARS.os= OPSYS") + "SUBST_CLASSES+=\tos", + "SUBST_STAGE.os=\tpre-configure", + "SUBST_FILES.os=\tguess-os.h", + "SUBST_VARS.os=\tPREFIX VARBASE") - // TODO: Warn that the conditional line is overwritten. t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -752,7 +1118,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) { // If the SUBST_CLASS identifier ends with a plus, the generated code must // use the correct assignment operator and be nicely formatted. -func (s *Suite) Test_SubstContext_suggestSubstVars__plus(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__plus(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -786,7 +1152,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__plus(c *check.C) { // The last of the SUBST_SED variables is 15 characters wide. When SUBST_SED // is replaced with SUBST_VARS, this becomes 16 characters and therefore // requires the whole paragraph to be indented by one more tab. -func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_realign_paragraph(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__autofix_realign_paragraph(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -818,7 +1184,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_realign_paragraph(c "SUBST_VARS.pfx+= PREFIX") } -func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_sed(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__autofix_plus_sed(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -849,7 +1215,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_sed(c *check.C) "SUBST_SED.pfx+= -e s,@PREFIX@,other,g") } -func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_vars(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__autofix_plus_vars(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -881,7 +1247,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_vars(c *check.C "SUBST_VARS.id+= PKGMANDIR") } -func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_indentation(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__autofix_indentation(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -911,7 +1277,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_indentation(c *check "SUBST_VARS.fix-paths= PREFIX") } -func (s *Suite) Test_SubstContext_suggestSubstVars__conditional(c *check.C) { +func (s *Suite) Test_substBlock_suggestSubstVars__conditional(c *check.C) { t := s.Init(c) t.Chdir(".") @@ -938,389 +1304,118 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__conditional(c *check.C) { "with \"SUBST_VARS.id+=\\tVAR2\".") t.CheckFileLinesDetab("filename.mk", - "SUBST_CLASSES+= id", - "SUBST_STAGE.id= pre-configure", - "SUBST_FILES.id= files", - "SUBST_VARS.id= VAR", - ".if 1", - "SUBST_VARS.id+= VAR2", - ".endif") -} - -func (s *Suite) Test_SubstContext_extractVarname(c *check.C) { - t := s.Init(c) - - test := func(input, expected string) { - t.CheckEquals((*SubstContext).extractVarname(nil, input), expected) - } - - // A simple variable name. - test("s,@VAR@,${VAR},", "VAR") - - // A parameterized variable name. - test("s,@VAR.param@,${VAR.param},", "VAR.param") - - // Only substitution commands can be replaced with SUBST_VARS. - test("/pattern/d", "") - - // An incomplete substitution command. - test("s", "") - - // Wrong placeholder character, only @ works. - test("s,!VAR!,${VAR},", "") - - // The placeholder must have exactly 1 @ on each side. - test("s,@@VAR@@,${VAR},", "") - - // Malformed because the comma is the separator. - test("s,@VAR,VAR@,${VAR},", "") - - // The replacement pattern is not a simple variable name enclosed in @. - test("s,@VAR!VAR@,${VAR},", "") - - // The replacement may only contain the :Q modifier. - test("s,@VAR@,${VAR:Mpattern},", "") - - // The :Q modifier is allowed in the replacement. - test("s,@VAR@,${VAR:Q},", "VAR") - - // The replacement may contain the :Q modifier only once. - test("s,@VAR@,${VAR:Q:Q},", "") - - // The replacement must be a plain variable expression, without prefix. - test("s,@VAR@,prefix${VAR},", "") - - // The replacement must be a plain variable expression, without suffix. - test("s,@VAR@,${VAR}suffix,", "") -} - -func (s *Suite) Test_SubstContext_directive__before_SUBST_CLASSES(c *check.C) { - t := s.Init(c) - - t.RunSubst( - ".if 0", - ".endif", - "SUBST_CLASSES+=\tos", - ".elif 0") // Just for branch coverage. - - t.CheckOutputLines( - "WARN: filename.mk:EOF: Missing SUBST block for \"os\".") -} - -func (s *Suite) Test_SubstContext_directive__conditional_blocks_complete(c *check.C) { - t := s.Init(c) - - t.RunSubst( - ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= nb", - "SUBST_STAGE.nb= post-configure", - "SUBST_FILES.nb= guess-netbsd.h", - "SUBST_VARS.nb= HAVE_NETBSD", - ".else", - "SUBST_CLASSES+= os", - "SUBST_STAGE.os= post-configure", - "SUBST_FILES.os= guess-netbsd.h", - "SUBST_VARS.os= HAVE_OTHER", - ".endif") - - t.CheckOutputEmpty() -} - -func (s *Suite) Test_SubstContext_directive__conditional_blocks_incomplete(c *check.C) { - t := s.Init(c) - - t.RunSubst( - ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= nb", - "SUBST_STAGE.nb= post-configure", - "SUBST_VARS.nb= HAVE_NETBSD", - ".else", - "SUBST_CLASSES+= os", - "SUBST_STAGE.os= post-configure", - "SUBST_FILES.os= guess-netbsd.h", - ".endif") - - t.CheckOutputLines( - "WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.nb missing.", - "WARN: filename.mk:9: Incomplete SUBST block: "+ - "SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") -} - -func (s *Suite) Test_SubstContext_directive__conditional_complete(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= id", - ".if ${OPSYS} == NetBSD", - "SUBST_STAGE.id=\t\tpost-configure", - "SUBST_MESSAGE.id=\tpost-configure", - "SUBST_FILES.id=\t\tguess-netbsd.h", - "SUBST_SED.id=\t\t-e s,from,to,", - "SUBST_VARS.id=\t\tHAVE_OTHER", - "SUBST_FILTER_CMD.id=\tHAVE_OTHER", - ".else", - "SUBST_STAGE.id=\t\tpost-configure", - "SUBST_MESSAGE.id=\tpost-configure", - "SUBST_FILES.id=\t\tguess-netbsd.h", - "SUBST_SED.id=\t\t-e s,from,to,", - "SUBST_VARS.id=\t\tHAVE_OTHER", - "SUBST_FILTER_CMD.id=\tHAVE_OTHER", - ".endif") - - t.CheckOutputLines( - "WARN: filename.mk:3: SUBST_STAGE.id should not be defined conditionally.", - "WARN: filename.mk:4: SUBST_MESSAGE.id should not be defined conditionally.", - "WARN: filename.mk:10: SUBST_STAGE.id should not be defined conditionally.", - "WARN: filename.mk:11: SUBST_MESSAGE.id should not be defined conditionally.") -} - -func (s *Suite) Test_SubstContext_directive__conditionally_overwritten_filter(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= id", - "SUBST_STAGE.id=\t\tpost-configure", - "SUBST_MESSAGE.id=\tpost-configure", - "SUBST_FILES.id=\t\tguess-netbsd.h", - "SUBST_FILTER_CMD.id=\tHAVE_OTHER", - ".if ${OPSYS} == NetBSD", - "SUBST_FILTER_CMD.id=\tHAVE_OTHER", - ".endif") - - t.CheckOutputLines( - "WARN: filename.mk:7: Duplicate definition of \"SUBST_FILTER_CMD.id\".") -} - -// Hopefully nobody will ever trigger this case in real pkgsrc. -// It's plain confusing to a casual reader to nest a complete -// SUBST block into another SUBST block. -// That's why pkglint doesn't cover this case correctly. -func (s *Suite) Test_SubstContext_directive__conditionally_nested_block(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= outer", - "SUBST_STAGE.outer= post-configure", - "SUBST_FILES.outer= outer.txt", - ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= inner", - "SUBST_STAGE.inner= post-configure", - "SUBST_FILES.inner= inner.txt", - "SUBST_VARS.inner= INNER", - ".endif", - "SUBST_VARS.outer= OUTER") - - t.CheckOutputLines( - "WARN: filename.mk:5: Incomplete SUBST block: "+ - "SUBST_SED.outer, SUBST_VARS.outer or SUBST_FILTER_CMD.outer missing.", - "WARN: filename.mk:5: Subst block \"outer\" should be finished "+ - "before adding the next class to SUBST_CLASSES.", - "WARN: filename.mk:10: "+ - "Late additions to a SUBST variable should use the += operator.") -} - -// It's completely valid to have several SUBST blocks in a single paragraph. -// As soon as a SUBST_CLASSES line appears, pkglint assumes that all previous -// SUBST blocks are finished. That's exactly the case here. -func (s *Suite) Test_SubstContext_directive__conditionally_following_block(c *check.C) { - t := s.Init(c) - - t.RunSubst( - "SUBST_CLASSES+= outer", - "SUBST_STAGE.outer= post-configure", - "SUBST_FILES.outer= outer.txt", - "SUBST_VARS.outer= OUTER", - ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= middle", - "SUBST_STAGE.middle= post-configure", - "SUBST_FILES.middle= inner.txt", - "SUBST_VARS.middle= INNER", - ". if ${MACHINE_ARCH} == amd64", - "SUBST_CLASSES+= inner", - "SUBST_STAGE.inner= post-configure", - "SUBST_FILES.inner= inner.txt", - "SUBST_VARS.inner= INNER", - ". endif", + "SUBST_CLASSES+= id", + "SUBST_STAGE.id= pre-configure", + "SUBST_FILES.id= files", + "SUBST_VARS.id= VAR", + ".if 1", + "SUBST_VARS.id+= VAR2", ".endif") - - t.CheckOutputEmpty() } -func (s *Suite) Test_SubstContext_directive__two_blocks_in_condition(c *check.C) { +func (s *Suite) Test_substBlock_dupList__late_addition(c *check.C) { t := s.Init(c) t.RunSubst( + "SUBST_CLASSES+=\tid", + "SUBST_STAGE.id=\tpost-configure", + "SUBST_FILES.id=\tfiles", + "SUBST_VARS.id=\tPREFIX", + "", ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= a", - "SUBST_STAGE.a= post-configure", - "SUBST_FILES.a= outer.txt", - "SUBST_VARS.a= OUTER", - "SUBST_CLASSES+= b", - "SUBST_STAGE.b= post-configure", - "SUBST_FILES.b= inner.txt", - "SUBST_VARS.b= INNER", + "SUBST_VARS.id=\tOPSYS", ".endif") - // Up to 2019-12-12, pkglint wrongly warned in filename.mk:6: - // Subst block "a" should be finished before adding - // the next class to SUBST_CLASSES. - // The warning was wrong since block "a" has all required fields set. - // The warning was caused by an inconsistent check whether the current - // block had any conditional variables. - t.CheckOutputEmpty() + t.CheckOutputLines( + "WARN: filename.mk:7: All but the first assignment " + + "to \"SUBST_VARS.id\" should use the \"+=\" operator.") } -func (s *Suite) Test_SubstContext_directive__nested_conditional_incomplete_block(c *check.C) { +func (s *Suite) Test_substBlock_dupList__conditional_before_unconditional(c *check.C) { t := s.Init(c) t.RunSubst( - "SUBST_CLASSES+= outer", - "SUBST_STAGE.outer= post-configure", - "SUBST_FILES.outer= outer.txt", - "SUBST_VARS.outer= OUTER", - ".if ${OPSYS} == NetBSD", - "SUBST_CLASSES+= inner1", - "SUBST_STAGE.inner1= post-configure", - "SUBST_VARS.inner1= INNER", - "SUBST_CLASSES+= inner2", - "SUBST_STAGE.inner2= post-configure", - "SUBST_FILES.inner2= inner.txt", - "SUBST_VARS.inner2= INNER", - ".endif") + "SUBST_CLASSES+= os", + "SUBST_STAGE.os= post-configure", + ".if 1", + "SUBST_FILES.os= conditional", + ".endif", + "SUBST_FILES.os= unconditional", + "SUBST_VARS.os= OPSYS") t.CheckOutputLines( - "WARN: filename.mk:9: Incomplete SUBST block: SUBST_FILES.inner1 missing.", - "WARN: filename.mk:9: Subst block \"inner1\" should be finished "+ - "before adding the next class to SUBST_CLASSES.") + "WARN: filename.mk:6: All but the first assignment " + + "to \"SUBST_FILES.os\" should use the \"+=\" operator.") } -func (s *Suite) Test_SubstContext_finishClass__details_in_then_branch(c *check.C) { +func (s *Suite) Test_substBlock_extractVarname(c *check.C) { t := s.Init(c) - t.RunSubst( - "SUBST_CLASSES+= os", - ".if ${OPSYS} == NetBSD", - "SUBST_VARS.os= OPSYS", - "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", - "SUBST_STAGE.os= post-configure", - "SUBST_MESSAGE.os= Guessing operating system", - "SUBST_FILES.os= guess-os.h", - ".endif") + test := func(input, expected string) { + t.CheckEquals((*substBlock).extractVarname(nil, input), expected) + } - t.CheckOutputLines( - "WARN: filename.mk:5: SUBST_STAGE.os should not be defined conditionally.", - "WARN: filename.mk:6: SUBST_MESSAGE.os should not be defined conditionally.", - "WARN: filename.mk:EOF: Missing SUBST block for \"os\".") -} + // A simple variable name. + test("s,@VAR@,${VAR},", "VAR") -func (s *Suite) Test_SubstContext_finishClass__details_in_else_branch(c *check.C) { - t := s.Init(c) + // A parameterized variable name. + test("s,@VAR.param@,${VAR.param},", "VAR.param") - t.RunSubst( - "SUBST_CLASSES+= os", - ".if ${OPSYS} == NetBSD", - ".else", - "SUBST_VARS.os= OPSYS", - "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", - "SUBST_STAGE.os= post-configure", - "SUBST_MESSAGE.os= Guessing operating system", - "SUBST_FILES.os= guess-os.h", - ".endif") + // Only substitution commands can be replaced with SUBST_VARS. + test("/pattern/d", "") - t.CheckOutputLines( - "WARN: filename.mk:6: SUBST_STAGE.os should not be defined conditionally.", - "WARN: filename.mk:7: SUBST_MESSAGE.os should not be defined conditionally.", - "WARN: filename.mk:EOF: Missing SUBST block for \"os\".") -} + // An incomplete substitution command. + test("s", "") -func (s *Suite) Test_SubstContext_finishClass__empty_conditional_at_end(c *check.C) { - t := s.Init(c) + // Wrong placeholder character, only @ works. + test("s,!VAR!,${VAR},", "") - t.RunSubst( - "SUBST_CLASSES+= os", - "SUBST_VARS.os= OPSYS", - "SUBST_SED.os= -e s,@OPSYS@,NetBSD,", - "SUBST_STAGE.os= post-configure", - "SUBST_MESSAGE.os= Guessing operating system", - "SUBST_FILES.os= guess-os.h", - ".if ${OPSYS} == NetBSD", - ".else", - ".endif") + // The placeholder must have exactly 1 @ on each side. + test("s,@@VAR@@,${VAR},", "") - t.CheckOutputEmpty() -} + // Malformed because the comma is the separator. + test("s,@VAR,VAR@,${VAR},", "") -func (s *Suite) Test_SubstContext_finishClass__missing_transformation_in_one_branch(c *check.C) { - t := s.Init(c) + // The replacement pattern is not a simple variable name enclosed in @. + test("s,@VAR!VAR@,${VAR},", "") - t.RunSubst( - "SUBST_CLASSES+= os", - "SUBST_STAGE.os= post-configure", - "SUBST_MESSAGE.os= Guessing operating system", - "SUBST_FILES.os= guess-os.h", - ".if ${OPSYS} == NetBSD", - "SUBST_FILES.os= -e s,@OpSYS@,NetBSD,", // A simple typo, this should be SUBST_SED. - ".elif ${OPSYS} == Darwin", - "SUBST_SED.os= -e s,@OPSYS@,Darwin1,", - "SUBST_SED.os= -e s,@OPSYS@,Darwin2,", - ".else", - "SUBST_VARS.os= OPSYS", - ".endif") + // The replacement may only contain the :Q modifier. + test("s,@VAR@,${VAR:Mpattern},", "") - t.CheckOutputLines( - "WARN: filename.mk:6: All but the first assignment "+ - "to \"SUBST_FILES.os\" should use the \"+=\" operator.", - "WARN: filename.mk:9: All but the first assignment "+ - "to \"SUBST_SED.os\" should use the \"+=\" operator.", - "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_SED.os, "+ - "SUBST_VARS.os or SUBST_FILTER_CMD.os missing.") -} + // The :Q modifier is allowed in the replacement. + test("s,@VAR@,${VAR:Q},", "VAR") -func (s *Suite) Test_SubstContext_finishClass__nested_conditionals(c *check.C) { - t := s.Init(c) + // The replacement may contain the :Q modifier only once. + test("s,@VAR@,${VAR:Q:Q},", "") - t.RunSubst( - "SUBST_CLASSES+= os", - "SUBST_STAGE.os= post-configure", - "SUBST_MESSAGE.os= Guessing operating system", - ".if ${OPSYS} == NetBSD", - "SUBST_FILES.os= guess-netbsd.h", - ". if ${ARCH} == i386", - "SUBST_FILTER_CMD.os= ${SED} -e s,@OPSYS,NetBSD-i386,", - ". elif ${ARCH} == x86_64", - "SUBST_VARS.os= OPSYS", - ". else", - "SUBST_SED.os= -e s,@OPSYS,NetBSD-unknown", - ". endif", - ".else", - // This branch omits SUBST_FILES. - "SUBST_SED.os= -e s,@OPSYS@,unknown,", - ".endif") + // The replacement must be a plain variable expression, without prefix. + test("s,@VAR@,prefix${VAR},", "") - t.CheckOutputLines( - "WARN: filename.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.") + // The replacement must be a plain variable expression, without suffix. + test("s,@VAR@,${VAR}suffix,", "") } -func (s *Suite) Test_SubstContext_isComplete__incomplete(c *check.C) { +func (s *Suite) Test_substBlock_isComplete__incomplete(c *check.C) { t := s.Init(c) ctx := NewSubstContext() ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0")) - t.CheckEquals(ctx.id, "") + t.CheckEquals(ctx.isActive(), false) ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=interp")) - t.CheckEquals(ctx.id, "interp") + t.CheckEquals(ctx.isActive(), false) ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.interp=Makefile")) - t.CheckEquals(ctx.isComplete(), false) + t.CheckEquals(ctx.activeId(), "interp") + t.CheckEquals(ctx.block().isComplete(), false) ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g")) - t.CheckEquals(ctx.isComplete(), false) + t.CheckEquals(ctx.block().isComplete(), false) ctx.Finish(t.NewMkLine("filename.mk", 14, "")) @@ -1330,7 +1425,7 @@ func (s *Suite) Test_SubstContext_isComplete__incomplete(c *check.C) { "WARN: filename.mk:14: Incomplete SUBST block: SUBST_STAGE.interp missing.") } -func (s *Suite) Test_SubstContext_isComplete__complete(c *check.C) { +func (s *Suite) Test_substBlock_isComplete__complete(c *check.C) { t := s.Init(c) ctx := NewSubstContext() @@ -1340,11 +1435,11 @@ func (s *Suite) Test_SubstContext_isComplete__complete(c *check.C) { ctx.varassign(t.NewMkLine("filename.mk", 12, "SUBST_FILES.p=Makefile")) ctx.varassign(t.NewMkLine("filename.mk", 13, "SUBST_SED.p=s,@PREFIX@,${PREFIX},g")) - t.CheckEquals(ctx.isComplete(), false) + t.CheckEquals(ctx.block().isComplete(), false) ctx.varassign(t.NewMkLine("filename.mk", 14, "SUBST_STAGE.p=post-configure")) - t.CheckEquals(ctx.isComplete(), true) + t.CheckEquals(ctx.block().isComplete(), true) ctx.Finish(t.NewMkLine("filename.mk", 15, "")) @@ -1352,3 +1447,106 @@ func (s *Suite) Test_SubstContext_isComplete__complete(c *check.C) { "NOTE: filename.mk:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" " + "can be replaced with \"SUBST_VARS.p= PREFIX\".") } + +func (s *Suite) Test_substBlock_finish__conditional_inside_then(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if ${OPSYS} == Linux", + "SUBST_CLASSES+=\tid", + "SUBST_STAGE.id=\tpre-configure", + "SUBST_SED.id=\t-e sahara", + ".else", + ".endif") + + // The block already ends at the .else, not at the end of the file, + // since that is the scope where the SUBST id is defined. + t.CheckOutputLines( + "WARN: filename.mk:5: Incomplete SUBST block: SUBST_FILES.id missing.") +} + +func (s *Suite) Test_substBlock_finish__conditional_inside_else(c *check.C) { + t := s.Init(c) + + t.RunSubst( + ".if ${OPSYS} == Linux", + ".else", + "SUBST_CLASSES+=\tid", + "SUBST_STAGE.id=\tpre-configure", + "SUBST_SED.id=\t-e sahara", + ".endif") + + // The block already ends at the .endif, not at the end of the file, + // since that is the scope where the SUBST id is defined. + t.CheckOutputLines( + "WARN: filename.mk:6: Incomplete SUBST block: SUBST_FILES.id missing.") +} + +func (s *Suite) Test_substBlock_finish__files_missing(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= one", + "SUBST_STAGE.one= pre-configure", + "SUBST_CLASSES+= two", + "SUBST_STAGE.two= pre-configure", + "SUBST_FILES.two= two.txt", + "SUBST_SED.two= s,two,2,g") + + t.CheckOutputLines( + "WARN: filename.mk:3: Subst block \"one\" should be finished "+ + "before adding the next class to SUBST_CLASSES.", + "WARN: filename.mk:3: Incomplete SUBST block: SUBST_FILES.one missing.", + "WARN: filename.mk:3: Incomplete SUBST block: "+ + "SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.") +} + +// Variables mentioned in SUBST_VARS may appear in the same paragraph, +// or alternatively anywhere else in the file. +func (s *Suite) Test_substBlock_checkForeignVariables__in_next_paragraph(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\tos", + "SUBST_STAGE.os=\tpre-configure", + "SUBST_FILES.os=\tguess-os.h", + "SUBST_VARS.os=\tTODAY1", + "", + "TODAY1!=\tdate", + "TODAY2!=\tdate") + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_substBlock_checkForeignVariables__mixed_separate(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+= 1", + "SUBST_STAGE.1= post-configure", + "SUBST_FILES.1= files", + "", + "SUBST_VARS.1= VAR", + "USE_TOOLS+= gmake") + + // The USE_TOOLS is not in the SUBST block anymore since there is + // an empty line between SUBST_CLASSES and SUBST_VARS. + t.CheckOutputEmpty() +} + +// Variables mentioned in SUBST_VARS are not considered "foreign" +// in the block and may be mixed with the other SUBST variables. +func (s *Suite) Test_substBlock_checkForeignVariables__in_block(c *check.C) { + t := s.Init(c) + + t.RunSubst( + "SUBST_CLASSES+=\tos", + "SUBST_STAGE.os=\tpre-configure", + "SUBST_FILES.os=\tguess-os.h", + "SUBST_VARS.os=\tTODAY1", + "TODAY1!=\tdate", + "TODAY2!=\tdate") + + t.CheckOutputLines( + "WARN: filename.mk:6: Foreign variable \"TODAY2\" in SUBST block.") +} diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 568f46c7a86..df471079ffd 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -518,7 +518,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { "openjdk8 oracle-jdk8 openjdk7 sun-jdk7 jdk16 jdk15 kaffe", "_PKG_JVMS.*") - platforms := reg.enumFromFiles(src, + operatingSystems := reg.enumFromFiles(src, "mk/platform", `(.*)\.mk$`, "$1", "Cygwin DragonFly FreeBSD Linux NetBSD SunOS") @@ -1362,7 +1362,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkglistrat("ONLY_FOR_COMPILER", compilers) reg.pkglistrat("ONLY_FOR_PLATFORM", BtMachinePlatformPattern) reg.pkgrat("ONLY_FOR_UNPRIVILEGED", BtYesNo) - reg.sysload("OPSYS", platforms, DefinedIfInScope|NonemptyIfDefined) + reg.sysload("OPSYS", operatingSystems, DefinedIfInScope|NonemptyIfDefined) reg.pkglistbl3("OPSYSVARS", BtVariableName) reg.pkg("OSVERSION_SPECIFIC", BtYes) reg.sysload("OS_VARIANT", BtIdentifierDirect, DefinedIfInScope) diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index 2bd90c889d6..f489251d4ef 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -467,7 +467,6 @@ var ( BtYesNo = &BasicType{"YesNo", (*VartypeCheck).YesNo} BtYesNoIndirectly = &BasicType{"YesNoIndirectly", (*VartypeCheck).YesNoIndirectly} - BtMachineOpsys = enumFromValues(machineOpsysValues) BtMachineArch = enumFromValues(machineArchValues) BtMachineGnuArch = enumFromValues(machineGnuArchValues) BtEmulOpsys = enumFromValues(emulOpsysValues) @@ -490,10 +489,6 @@ func init() { // TODO: Move these values to VarTypeRegistry.Init and read them from the // pkgsrc infrastructure files, as far as possible. const ( - machineOpsysValues = "" + // See mk/platform - "AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " + - "HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare" - // See mk/emulator/emulator-vars.mk. emulOpsysValues = "" + "bitrig bsdos cygwin darwin dragonfly freebsd " + diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 399c82253f5..d2a4f407472 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -799,8 +799,14 @@ func (cv *VartypeCheck) MachinePlatformPattern() { } if m, opsysPattern, versionPattern, archPattern := match3(pattern, reTriple); m { + // This is cheated, but the dynamically loaded enums are not + // provided in a type registry where they could be looked up + // by a name. The following line therefore assumes that OPSYS + // is an operating system name, which sounds like a safe bet. + btOpsys := G.Pkgsrc.vartypes.types["OPSYS"].basicType + opsysCv := cv.WithVarnameValueMatch("the operating system part of "+cv.Varname, opsysPattern) - BtMachineOpsys.checker(opsysCv) + btOpsys.checker(opsysCv) versionCv := cv.WithVarnameValueMatch("the version part of "+cv.Varname, versionPattern) versionCv.Version() diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index da05d89357b..d80ecdd6153 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -1176,8 +1176,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) { vt.Output( "WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.", "WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+ - "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ - "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+ "} for the operating system part of MACHINE_PLATFORM.", "WARN: filename.mk:2: The pattern \"8087\" cannot match any of "+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+ @@ -1191,8 +1190,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) { "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+ "} for the hardware architecture part of MACHINE_PLATFORM.", "WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+ - "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ - "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+ "} for the operating system part of MACHINE_PLATFORM.", "WARN: filename.mk:3: The pattern \"l*\" cannot match any of "+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+ @@ -1227,8 +1225,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) { vt.Output( "WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.", "WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+ - "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ - "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+ "} for the operating system part of ONLY_FOR_PLATFORM.", "WARN: filename.mk:2: The pattern \"8087\" cannot match any of "+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+ @@ -1242,8 +1239,7 @@ func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) { "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+ "} for the hardware architecture part of ONLY_FOR_PLATFORM.", "WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+ - "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ - "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS "+ "} for the operating system part of ONLY_FOR_PLATFORM.", "WARN: filename.mk:3: The pattern \"l*\" cannot match any of "+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+ -- cgit v1.2.3