From d679785d0d52526e1f86580072771aabfc466f6c Mon Sep 17 00:00:00 2001 From: Sasha Smundak Date: Mon, 15 Nov 2021 13:01:53 -0800 Subject: [PATCH 1/2] Convert dist-for-goals. Bug: 198496782 Test: internal Change-Id: I64ae938a5809238c18aca272ba73e4328fcb9efe --- mk2rbc/mk2rbc.go | 1 + mk2rbc/mk2rbc_test.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index b87ab4171..ee6cf792d 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -102,6 +102,7 @@ var knownFunctions = map[string]struct { "addsuffix": {baseName + ".addsuffix", starlarkTypeList, hiddenArgNone}, "copy-files": {baseName + ".copy_files", starlarkTypeList, hiddenArgNone}, "dir": {baseName + ".dir", starlarkTypeList, hiddenArgNone}, + "dist-for-goals": {baseName + ".mkdist_for_goals", starlarkTypeVoid, hiddenArgGlobal}, "enforce-product-packages-exist": {baseName + ".enforce_product_packages_exist", starlarkTypeVoid, hiddenArgNone}, "error": {baseName + ".mkerror", starlarkTypeVoid, hiddenArgNone}, "findstring": {"!findstring", starlarkTypeInt, hiddenArgNone}, diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index c2c46543e..41c32d1ea 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -689,6 +689,7 @@ $(call enforce-product-packages-exist,) $(call enforce-product-packages-exist, foo) $(call require-artifacts-in-path, foo, bar) $(call require-artifacts-in-path-relaxed, foo, bar) +$(call dist-for-goals, goal, from:to) `, expected: `load("//build/make/core:product_config.rbc", "rblf") @@ -698,6 +699,7 @@ def init(g, handle): rblf.enforce_product_packages_exist("foo") rblf.require_artifacts_in_path("foo", "bar") rblf.require_artifacts_in_path_relaxed("foo", "bar") + rblf.mkdist_for_goals(g, "goal", "from:to") `, }, { From 422b614355c0dcbad5d4b989d52fa0b1db252821 Mon Sep 17 00:00:00 2001 From: Sasha Smundak Date: Thu, 11 Nov 2021 18:31:59 -0800 Subject: [PATCH 2/2] Generate runtime conversion diagnostics Instead of inserting a comment with error description into the generated code, generate a call to a function that will print out a conversion error when executed. Do not print generic "partially converted" message anymore. Remove --verbose and --no_warnings options. Bug: 204062171 Test: internal Change-Id: Ib126e16dc76f49635e4939e670922f2561781049 --- mk2rbc/cmd/mk2rbc.go | 64 ++++++++++++---------------- mk2rbc/expr.go | 20 ++++----- mk2rbc/mk2rbc.go | 98 +++++++++++++++++++++++-------------------- mk2rbc/mk2rbc_test.go | 67 ++++++++++++++--------------- mk2rbc/node.go | 13 +----- mk2rbc/variable.go | 7 ++-- 6 files changed, 123 insertions(+), 146 deletions(-) diff --git a/mk2rbc/cmd/mk2rbc.go b/mk2rbc/cmd/mk2rbc.go index 7bb0246be..bb5a68033 100644 --- a/mk2rbc/cmd/mk2rbc.go +++ b/mk2rbc/cmd/mk2rbc.go @@ -46,8 +46,6 @@ var ( dryRun = flag.Bool("dry_run", false, "dry run") recurse = flag.Bool("convert_dependents", false, "convert all dependent files") mode = flag.String("mode", "", `"backup" to back up existing files, "write" to overwrite them`) - noWarn = flag.Bool("no_warnings", false, "don't warn about partially failed conversions") - verbose = flag.Bool("v", false, "print summary") errstat = flag.Bool("error_stat", false, "print error statistics") traceVar = flag.String("trace", "", "comma-separated list of variables to trace") // TODO(asmundak): this option is for debugging @@ -75,7 +73,6 @@ func init() { flagAlias("root", "d") flagAlias("dry_run", "n") flagAlias("convert_dependents", "r") - flagAlias("no_warnings", "w") flagAlias("error_stat", "e") } @@ -207,9 +204,9 @@ func main() { } } - printStats() if *errstat { errorLogger.printStatistics() + printStats() } if !ok { os.Exit(1) @@ -329,17 +326,16 @@ func convertOne(mkFile string) (ok bool) { }() mk2starRequest := mk2rbc.Request{ - MkFile: mkFile, - Reader: nil, - RootDir: *rootDir, - OutputDir: *outputTop, - OutputSuffix: *suffix, - TracedVariables: tracedVariables, - TraceCalls: *traceCalls, - WarnPartialSuccess: !*noWarn, - SourceFS: os.DirFS(*rootDir), - MakefileFinder: makefileFinder, - ErrorLogger: errorLogger, + MkFile: mkFile, + Reader: nil, + RootDir: *rootDir, + OutputDir: *outputTop, + OutputSuffix: *suffix, + TracedVariables: tracedVariables, + TraceCalls: *traceCalls, + SourceFS: os.DirFS(*rootDir), + MakefileFinder: makefileFinder, + ErrorLogger: errorLogger, } ss, err := mk2rbc.Convert(mk2starRequest) if err != nil { @@ -417,9 +413,6 @@ func writeGenerated(path string, contents string) error { func printStats() { var sortedFiles []string - if *noWarn && !*verbose { - return - } for p := range converted { sortedFiles = append(sortedFiles, p) } @@ -435,27 +428,22 @@ func printStats() { nOk++ } } - if !*noWarn { - if nPartial > 0 { - fmt.Fprintf(os.Stderr, "Conversion was partially successful for:\n") - for _, f := range sortedFiles { - if ss := converted[f]; ss != nil && ss.HasErrors() { - fmt.Fprintln(os.Stderr, " ", f) - } - } - } - - if nFailed > 0 { - fmt.Fprintf(os.Stderr, "Conversion failed for files:\n") - for _, f := range sortedFiles { - if converted[f] == nil { - fmt.Fprintln(os.Stderr, " ", f) - } + if nPartial > 0 { + fmt.Fprintf(os.Stderr, "Conversion was partially successful for:\n") + for _, f := range sortedFiles { + if ss := converted[f]; ss != nil && ss.HasErrors() { + fmt.Fprintln(os.Stderr, " ", f) } } } - if *verbose && (nPartial > 0 || nFailed > 0) { - fmt.Fprintln(os.Stderr, "Succeeded: ", nOk, " Partial: ", nPartial, " Failed: ", nFailed) + + if nFailed > 0 { + fmt.Fprintf(os.Stderr, "Conversion failed for files:\n") + for _, f := range sortedFiles { + if converted[f] == nil { + fmt.Fprintln(os.Stderr, " ", f) + } + } } } @@ -468,8 +456,8 @@ type errorSink struct { data map[string]datum } -func (ebt errorSink) NewError(sourceFile string, sourceLine int, node parser.Node, message string, args ...interface{}) { - fmt.Fprintf(os.Stderr, "%s:%d ", sourceFile, sourceLine) +func (ebt errorSink) NewError(el mk2rbc.ErrorLocation, node parser.Node, message string, args ...interface{}) { + fmt.Fprint(os.Stderr, el, ": ") fmt.Fprintf(os.Stderr, message, args...) fmt.Fprintln(os.Stderr) if !*errstat { diff --git a/mk2rbc/expr.go b/mk2rbc/expr.go index 8279d2e17..65fd6f7c0 100644 --- a/mk2rbc/expr.go +++ b/mk2rbc/expr.go @@ -18,8 +18,6 @@ import ( "fmt" "strconv" "strings" - - mkparser "android/soong/androidmk/parser" ) // Represents an expression in the Starlark code. An expression has @@ -106,7 +104,7 @@ func (xi *interpolateExpr) emit(gctx *generationContext) { format += "%s" + strings.ReplaceAll(chunk, "%", "%%") } gctx.writef("%q %% ", format) - emitarg := func(arg starlarkExpr) { + emitArg := func(arg starlarkExpr) { if arg.typ() == starlarkTypeList { gctx.write(`" ".join(`) arg.emit(gctx) @@ -116,12 +114,12 @@ func (xi *interpolateExpr) emit(gctx *generationContext) { } } if len(xi.args) == 1 { - emitarg(xi.args[0]) + emitArg(xi.args[0]) } else { sep := "(" for _, arg := range xi.args { gctx.write(sep) - emitarg(arg) + emitArg(arg) sep = ", " } gctx.write(")") @@ -414,7 +412,7 @@ func newStringListExpr(items []string) *listExpr { return &v } -// concatExpr generates epxr1 + expr2 + ... + exprN in Starlark. +// concatExpr generates expr1 + expr2 + ... + exprN in Starlark. type concatExpr struct { items []starlarkExpr } @@ -607,8 +605,8 @@ func (cx *callExpr) emitListVarCopy(gctx *generationContext) { } type badExpr struct { - node mkparser.Node - message string + errorLocation ErrorLocation + message string } func (b *badExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) { @@ -617,15 +615,15 @@ func (b *badExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) return } -func (b *badExpr) emit(_ *generationContext) { - panic("implement me") +func (b *badExpr) emit(gctx *generationContext) { + gctx.emitConversionError(b.errorLocation, b.message) } func (_ *badExpr) typ() starlarkType { return starlarkTypeUnknown } -func (b *badExpr) emitListVarCopy(gctx *generationContext) { +func (_ *badExpr) emitListVarCopy(_ *generationContext) { panic("implement me") } diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index ee6cf792d..c55300e34 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -157,23 +157,31 @@ var builtinFuncRex = regexp.MustCompile( // Conversion request parameters type Request struct { - MkFile string // file to convert - Reader io.Reader // if set, read input from this stream instead - RootDir string // root directory path used to resolve included files - OutputSuffix string // generated Starlark files suffix - OutputDir string // if set, root of the output hierarchy - ErrorLogger ErrorLogger - TracedVariables []string // trace assignment to these variables - TraceCalls bool - WarnPartialSuccess bool - SourceFS fs.FS - MakefileFinder MakefileFinder + MkFile string // file to convert + Reader io.Reader // if set, read input from this stream instead + RootDir string // root directory path used to resolve included files + OutputSuffix string // generated Starlark files suffix + OutputDir string // if set, root of the output hierarchy + ErrorLogger ErrorLogger + TracedVariables []string // trace assignment to these variables + TraceCalls bool + SourceFS fs.FS + MakefileFinder MakefileFinder } // ErrorLogger prints errors and gathers error statistics. // Its NewError function is called on every error encountered during the conversion. type ErrorLogger interface { - NewError(sourceFile string, sourceLine int, node mkparser.Node, text string, args ...interface{}) + NewError(el ErrorLocation, node mkparser.Node, text string, args ...interface{}) +} + +type ErrorLocation struct { + MkFile string + MkLine int +} + +func (el ErrorLocation) String() string { + return fmt.Sprintf("%s:%d", el.MkFile, el.MkLine) } // Derives module name for a given file. It is base name @@ -249,10 +257,6 @@ func (gctx *generationContext) emit() string { node.emit(gctx) } - if ss.hasErrors && ss.warnPartialSuccess { - gctx.newLine() - gctx.writef("%s(%q, %q)", cfnWarning, filepath.Base(ss.mkFile), "partially successful conversion") - } if gctx.starScript.traceCalls { gctx.newLine() gctx.writef(`print("<%s")`, gctx.starScript.mkFile) @@ -307,6 +311,10 @@ func (gctx *generationContext) newLine() { gctx.writef("%*s", 2*gctx.indentLevel, "") } +func (gctx *generationContext) emitConversionError(el ErrorLocation, message string) { + gctx.writef(`rblf.mk2rbc_error("%s", %q)`, el, message) +} + type knownVariable struct { name string class varClass @@ -371,18 +379,17 @@ type nodeReceiver interface { // Information about the generated Starlark script. type StarlarkScript struct { - mkFile string - moduleName string - mkPos scanner.Position - nodes []starlarkNode - inherited []*moduleInfo - hasErrors bool - topDir string - traceCalls bool // print enter/exit each init function - warnPartialSuccess bool - sourceFS fs.FS - makefileFinder MakefileFinder - nodeLocator func(pos mkparser.Pos) int + mkFile string + moduleName string + mkPos scanner.Position + nodes []starlarkNode + inherited []*moduleInfo + hasErrors bool + topDir string + traceCalls bool // print enter/exit each init function + sourceFS fs.FS + makefileFinder MakefileFinder + nodeLocator func(pos mkparser.Pos) int } func (ss *StarlarkScript) newNode(node starlarkNode) { @@ -561,7 +568,7 @@ func (ctx *parseContext) handleAssignment(a *mkparser.Assignment) { return } _, isTraced := ctx.tracedVariables[name] - asgn := &assignmentNode{lhs: lhs, mkValue: a.Value, isTraced: isTraced} + asgn := &assignmentNode{lhs: lhs, mkValue: a.Value, isTraced: isTraced, location: ctx.errorLocation(a)} if lhs.valueType() == starlarkTypeUnknown { // Try to divine variable type from the RHS asgn.value = ctx.parseMakeString(a, a.Value) @@ -1024,10 +1031,10 @@ func (ctx *parseContext) parseCondition(check *mkparser.Directive) starlarkNode func (ctx *parseContext) newBadExpr(node mkparser.Node, text string, args ...interface{}) starlarkExpr { message := fmt.Sprintf(text, args...) if ctx.errorLogger != nil { - ctx.errorLogger.NewError(ctx.script.mkFile, ctx.script.nodeLocator(node.Pos()), node, text, args...) + ctx.errorLogger.NewError(ctx.errorLocation(node), node, text, args...) } ctx.script.hasErrors = true - return &badExpr{node, message} + return &badExpr{errorLocation: ctx.errorLocation(node), message: message} } func (ctx *parseContext) parseCompare(cond *mkparser.Directive) starlarkExpr { @@ -1545,18 +1552,14 @@ func (ctx *parseContext) carryAsComment(failedNode mkparser.Node) { // records that the given node failed to be converted and includes an explanatory message func (ctx *parseContext) errorf(failedNode mkparser.Node, message string, args ...interface{}) { if ctx.errorLogger != nil { - ctx.errorLogger.NewError(ctx.script.mkFile, ctx.script.nodeLocator(failedNode.Pos()), failedNode, message, args...) + ctx.errorLogger.NewError(ctx.errorLocation(failedNode), failedNode, message, args...) } - message = fmt.Sprintf(message, args...) - ctx.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", message)) - ctx.carryAsComment(failedNode) - + ctx.receiver.newNode(&exprNode{ctx.newBadExpr(failedNode, message, args...)}) ctx.script.hasErrors = true } func (ctx *parseContext) wrapBadExpr(xBad *badExpr) { - ctx.insertComment(fmt.Sprintf("# MK2RBC TRANSLATION ERROR: %s", xBad.message)) - ctx.carryAsComment(xBad.node) + ctx.receiver.newNode(&exprNode{xBad}) } func (ctx *parseContext) loadedModulePath(path string) string { @@ -1622,6 +1625,10 @@ func (ctx *parseContext) hasNamespaceVar(namespaceName string, varName string) b return ok } +func (ctx *parseContext) errorLocation(node mkparser.Node) ErrorLocation { + return ErrorLocation{ctx.script.mkFile, ctx.script.nodeLocator(node.Pos())} +} + func (ss *StarlarkScript) String() string { return NewGenerateContext(ss).emit() } @@ -1660,14 +1667,13 @@ func Convert(req Request) (*StarlarkScript, error) { return nil, fmt.Errorf("bad makefile %s", req.MkFile) } starScript := &StarlarkScript{ - moduleName: moduleNameForFile(req.MkFile), - mkFile: req.MkFile, - topDir: req.RootDir, - traceCalls: req.TraceCalls, - warnPartialSuccess: req.WarnPartialSuccess, - sourceFS: req.SourceFS, - makefileFinder: req.MakefileFinder, - nodeLocator: func(pos mkparser.Pos) int { return parser.Unpack(pos).Line }, + moduleName: moduleNameForFile(req.MkFile), + mkFile: req.MkFile, + topDir: req.RootDir, + traceCalls: req.TraceCalls, + sourceFS: req.SourceFS, + makefileFinder: req.MakefileFinder, + nodeLocator: func(pos mkparser.Pos) int { return parser.Unpack(pos).Line }, } ctx := newParseContext(starScript, nodes) ctx.outputSuffix = req.OutputSuffix diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index 41c32d1ea..22490a2f5 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -105,15 +105,12 @@ def init(g, handle): PRODUCT_NAME := $(call foo1, bar) PRODUCT_NAME := $(call foo0) `, - expected: `# MK2RBC TRANSLATION ERROR: cannot handle invoking foo1 -# PRODUCT_NAME := $(call foo1, bar) -# MK2RBC TRANSLATION ERROR: cannot handle invoking foo0 -# PRODUCT_NAME := $(call foo0) -load("//build/make/core:product_config.rbc", "rblf") + expected: `load("//build/make/core:product_config.rbc", "rblf") def init(g, handle): cfg = rblf.cfg(handle) - rblf.warning("product.mk", "partially successful conversion") + rblf.mk2rbc_error("product.mk:2", "cannot handle invoking foo1") + rblf.mk2rbc_error("product.mk:3", "cannot handle invoking foo0") `, }, { @@ -207,15 +204,11 @@ define some-macro $(info foo) endef `, - expected: `# MK2RBC TRANSLATION ERROR: define is not supported: some-macro -# define some-macro -# $(info foo) -# endef -load("//build/make/core:product_config.rbc", "rblf") + expected: `load("//build/make/core:product_config.rbc", "rblf") def init(g, handle): cfg = rblf.cfg(handle) - rblf.warning("product.mk", "partially successful conversion") + rblf.mk2rbc_error("product.mk:2", "define is not supported: some-macro") `, }, { @@ -282,9 +275,7 @@ def init(g, handle): # Comment pass else: - # MK2RBC TRANSLATION ERROR: cannot set predefined variable TARGET_COPY_OUT_RECOVERY to "foo", its value should be "recovery" - pass - rblf.warning("product.mk", "partially successful conversion") + rblf.mk2rbc_error("product.mk:5", "cannot set predefined variable TARGET_COPY_OUT_RECOVERY to \"foo\", its value should be \"recovery\"") `, }, { @@ -859,9 +850,7 @@ def init(g, handle): rblf.soong_config_namespace(g, "cvd") rblf.soong_config_set(g, "cvd", "launch_configs", "cvd_config_auto.json") rblf.soong_config_append(g, "cvd", "grub_config", "grub.cfg") - # MK2RBC TRANSLATION ERROR: SOONG_CONFIG_ variables cannot be referenced, use soong_config_get instead: SOONG_CONFIG_cvd_grub_config - # x := $(SOONG_CONFIG_cvd_grub_config) - rblf.warning("product.mk", "partially successful conversion") + rblf.mk2rbc_error("product.mk:7", "SOONG_CONFIG_ variables cannot be referenced, use soong_config_get instead: SOONG_CONFIG_cvd_grub_config") `, }, { desc: "soong namespace accesses", @@ -1048,15 +1037,11 @@ def init(g, handle): in: ` foo: foo.c gcc -o $@ $*`, - expected: `# MK2RBC TRANSLATION ERROR: unsupported line rule: foo: foo.c -#gcc -o $@ $* -# rule: foo: foo.c -# gcc -o $@ $* -load("//build/make/core:product_config.rbc", "rblf") + expected: `load("//build/make/core:product_config.rbc", "rblf") def init(g, handle): cfg = rblf.cfg(handle) - rblf.warning("product.mk", "partially successful conversion") + rblf.mk2rbc_error("product.mk:2", "unsupported line rule: foo: foo.c\n#gcc -o $@ $*") `, }, { @@ -1064,14 +1049,27 @@ def init(g, handle): mkname: "product.mk", in: ` override FOO:=`, - expected: `# MK2RBC TRANSLATION ERROR: cannot handle override directive -# override FOO := -load("//build/make/core:product_config.rbc", "rblf") + expected: `load("//build/make/core:product_config.rbc", "rblf") def init(g, handle): cfg = rblf.cfg(handle) + rblf.mk2rbc_error("product.mk:2", "cannot handle override directive") g["override FOO"] = "" - rblf.warning("product.mk", "partially successful conversion") +`, + }, + { + desc: "Bad expression", + mkname: "build/product.mk", + in: ` +ifeq (,$(call foobar)) +endif +`, + expected: `load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + if rblf.mk2rbc_error("build/product.mk:2", "cannot handle invoking foobar"): + pass `, }, } @@ -1142,13 +1140,12 @@ func TestGood(t *testing.T) { t.Run(test.desc, func(t *testing.T) { ss, err := Convert(Request{ - MkFile: test.mkname, - Reader: bytes.NewBufferString(test.in), - RootDir: ".", - OutputSuffix: ".star", - WarnPartialSuccess: true, - SourceFS: fs, - MakefileFinder: &testMakefileFinder{fs: fs}, + MkFile: test.mkname, + Reader: bytes.NewBufferString(test.in), + RootDir: ".", + OutputSuffix: ".star", + SourceFS: fs, + MakefileFinder: &testMakefileFinder{fs: fs}, }) if err != nil { t.Error(err) diff --git a/mk2rbc/node.go b/mk2rbc/node.go index 3fe1a17e6..d38299d34 100644 --- a/mk2rbc/node.go +++ b/mk2rbc/node.go @@ -183,6 +183,7 @@ type assignmentNode struct { value starlarkExpr mkValue *mkparser.MakeString flavor assignmentFlavor + location ErrorLocation isTraced bool previous *assignmentNode } @@ -223,18 +224,6 @@ func (in *ifNode) emit(gctx *generationContext) { } gctx.newLine() - if bad, ok := in.expr.(*badExpr); ok { - gctx.write("# MK2STAR ERROR converting:") - gctx.newLine() - gctx.writef("# %s", bad.node.Dump()) - gctx.newLine() - gctx.writef("# %s", bad.message) - gctx.newLine() - // The init function emits a warning if the conversion was not - // fullly successful, so here we (arbitrarily) take the false path. - gctx.writef("%sFalse:", ifElif) - return - } gctx.write(ifElif) in.expr.emit(gctx) gctx.write(":") diff --git a/mk2rbc/variable.go b/mk2rbc/variable.go index ded07fed6..6b67a7cb9 100644 --- a/mk2rbc/variable.go +++ b/mk2rbc/variable.go @@ -228,10 +228,9 @@ func (pv predefinedVariable) emitSet(gctx *generationContext, asgn *assignmentNo if actualValue == expectedValue { return } - gctx.writef("# MK2RBC TRANSLATION ERROR: cannot set predefined variable %s to %q, its value should be %q", - pv.name(), actualValue, expectedValue) - gctx.newLine() - gctx.write("pass") + gctx.emitConversionError(asgn.location, + fmt.Sprintf("cannot set predefined variable %s to %q, its value should be %q", + pv.name(), actualValue, expectedValue)) gctx.starScript.hasErrors = true return }