Repository: incubator-mynewt-newt Updated Branches: refs/heads/develop 744a4c152 -> d7eea66d9
MYNEWT-556 Supports detection of Priority Violation 3 factors contributed to newt ignoring the override silenty. 1) The code only checked for Lateral override 2) The code processed syscfg.defs and syscfg.vals by package priority (lowest first) So when a package overrides the setting of higher priority the setting definition has not been processed yet, and was treated as overriding an undefined setting, which is treated as a warning. 3) newt build did not print out warning message for overriding an undefined setting (See MYNEWT-557) Fix involves: 1) Process all the syscfg.defs for all package first. Then process the syscfg.vals for each package. 2) Save the package that defined the setting in CfgEntry 3) Create CfgPriority to save priority violation: setting name, package that defined the setting, and package that override the setting. 4) Reordered PackageType definition by package priority (in increasing order of priority). Also reoreded PackageTypeNames to match order of PackageType definition (not functionally needed) 5) Compare priority types between the package overriding the value and the package defining the setting. to detect prioirty violations. Newt aborts the build. Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/commit/962ae740 Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/tree/962ae740 Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/diff/962ae740 Branch: refs/heads/develop Commit: 962ae740a90d25780a86bc3d12ede46196c99426 Parents: 744a4c1 Author: cwanda <[email protected]> Authored: Wed Jan 25 20:31:04 2017 -0800 Committer: cwanda <[email protected]> Committed: Wed Jan 25 20:31:04 2017 -0800 ---------------------------------------------------------------------- newt/pkg/package.go | 25 ++++++------ newt/syscfg/syscfg.go | 97 ++++++++++++++++++++++++++++------------------ 2 files changed, 72 insertions(+), 50 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/blob/962ae740/newt/pkg/package.go ---------------------------------------------------------------------- diff --git a/newt/pkg/package.go b/newt/pkg/package.go index 5087c98..03795cb 100644 --- a/newt/pkg/package.go +++ b/newt/pkg/package.go @@ -30,28 +30,29 @@ const ( PACKAGE_STABILITY_DEV = "dev" ) +// Define constants with values of increasing priority. const ( - PACKAGE_TYPE_APP interfaces.PackageType = iota - PACKAGE_TYPE_BSP + PACKAGE_TYPE_COMPILER interfaces.PackageType = iota + PACKAGE_TYPE_MFG PACKAGE_TYPE_SDK - PACKAGE_TYPE_COMPILER + PACKAGE_TYPE_GENERATED PACKAGE_TYPE_LIB - PACKAGE_TYPE_TARGET + PACKAGE_TYPE_BSP PACKAGE_TYPE_UNITTEST - PACKAGE_TYPE_GENERATED - PACKAGE_TYPE_MFG + PACKAGE_TYPE_APP + PACKAGE_TYPE_TARGET ) var PackageTypeNames = map[interfaces.PackageType]string{ - PACKAGE_TYPE_APP: "app", - PACKAGE_TYPE_BSP: "bsp", - PACKAGE_TYPE_SDK: "sdk", PACKAGE_TYPE_COMPILER: "compiler", + PACKAGE_TYPE_MFG: "mfg", + PACKAGE_TYPE_SDK: "sdk", + PACKAGE_TYPE_GENERATED: "generated", PACKAGE_TYPE_LIB: "lib", - PACKAGE_TYPE_TARGET: "target", + PACKAGE_TYPE_BSP: "bsp", PACKAGE_TYPE_UNITTEST: "unittest", - PACKAGE_TYPE_GENERATED: "generated", - PACKAGE_TYPE_MFG: "mfg", + PACKAGE_TYPE_APP: "app", + PACKAGE_TYPE_TARGET: "target", } // An interface, representing information about a Package http://git-wip-us.apache.org/repos/asf/incubator-mynewt-newt/blob/962ae740/newt/syscfg/syscfg.go ---------------------------------------------------------------------- diff --git a/newt/syscfg/syscfg.go b/newt/syscfg/syscfg.go index 0010f56..1778b24 100644 --- a/newt/syscfg/syscfg.go +++ b/newt/syscfg/syscfg.go @@ -80,13 +80,14 @@ type CfgEntry struct { Description string SettingType CfgSettingType Restrictions []CfgRestriction - - History []CfgPoint + PackageDef *pkg.LocalPackage + History []CfgPoint } -type CfgLateral struct { - PkgName string +type CfgPriority struct { SettingName string + PackageDef *pkg.LocalPackage // package that define the setting. + PackageSrc *pkg.LocalPackage // package overriding setting value. } type CfgFlashConflictCode int @@ -116,19 +117,19 @@ type Cfg struct { Violations map[string][]CfgRestriction // Attempted override by bottom-priority packages (libraries). - Laterals []CfgLateral + PriorityViolations []CfgPriority FlashConflicts []CfgFlashConflict } func NewCfg() Cfg { return Cfg{ - Settings: map[string]CfgEntry{}, - Orphans: map[string][]CfgPoint{}, - Ambiguities: map[string][]CfgPoint{}, - Violations: map[string][]CfgRestriction{}, - Laterals: []CfgLateral{}, - FlashConflicts: []CfgFlashConflict{}, + Settings: map[string]CfgEntry{}, + Orphans: map[string][]CfgPoint{}, + Ambiguities: map[string][]CfgPoint{}, + Violations: map[string][]CfgRestriction{}, + PriorityViolations: []CfgPriority{}, + FlashConflicts: []CfgFlashConflict{}, } } @@ -284,6 +285,7 @@ func readSetting(name string, lpkg *pkg.LocalPackage, entry.Name = name entry.Description = stringValue(vals["description"]) entry.Value = stringValue(vals["value"]) + entry.PackageDef = lpkg if vals["type"] == nil { entry.SettingType = CFG_SETTING_TYPE_RAW } else { @@ -360,26 +362,28 @@ func (cfg *Cfg) readValsOnce(lpkg *pkg.LocalPackage, values := newtutil.GetStringMapFeatures(v, lfeatures, "syscfg.vals") for k, v := range values { - if normalizePkgType(lpkg.Type()) == pkg.PACKAGE_TYPE_LIB { - // A library package is overriding a setting; this is disallowed. - // Overrides must come from a higher priority package. - lateral := CfgLateral{ - PkgName: lpkg.Name(), - SettingName: k, - } - cfg.Laterals = append(cfg.Laterals, lateral) - } else { - entry, ok := cfg.Settings[k] - if ok { + entry, ok := cfg.Settings[k] + if ok { + sourcetype := normalizePkgType(lpkg.Type()) + deftype := normalizePkgType(entry.PackageDef.Type()) + if sourcetype <= deftype { + // Overrides must come from a higher priority package. + priority := CfgPriority{ + PackageDef: entry.PackageDef, + PackageSrc: lpkg, + SettingName: k, + } + cfg.PriorityViolations = append(cfg.PriorityViolations, priority) + } else { entry.appendValue(lpkg, v) cfg.Settings[k] = entry - } else { - orphan := CfgPoint{ - Value: stringValue(v), - Source: lpkg, - } - cfg.Orphans[k] = append(cfg.Orphans[k], orphan) } + } else { + orphan := CfgPoint{ + Value: stringValue(v), + Source: lpkg, + } + cfg.Orphans[k] = append(cfg.Orphans[k], orphan) } } @@ -551,15 +555,15 @@ func (cfg *Cfg) ErrorText() string { } } - if len(cfg.Laterals) > 0 { - str += "Lateral overrides detected (bottom-priority packages " + - "cannot override settings):\n" - for _, lateral := range cfg.Laterals { - entry := cfg.Settings[lateral.SettingName] - historyMap[lateral.SettingName] = entry.History + if len(cfg.PriorityViolations) > 0 { + str += "Priority violations detected (Packages can only override " + + "settings defined by packages of lower priority):\n" + for _, priority := range cfg.PriorityViolations { + entry := cfg.Settings[priority.SettingName] + historyMap[priority.SettingName] = entry.History - str += fmt.Sprintf(" Package: %s, Setting: %s\n", - lateral.PkgName, lateral.SettingName) + str += fmt.Sprintf(" Package: %s overriding setting: %s defined by %s\n", + priority.PackageSrc.Name(), priority.SettingName, priority.PackageDef.Name()) } } @@ -660,7 +664,7 @@ func categorizePkgs( return pmap } -func (cfg *Cfg) readForPkgType(lpkgs []*pkg.LocalPackage, +func (cfg *Cfg) readDefsForPkgType(lpkgs []*pkg.LocalPackage, features map[string]bool) error { for _, lpkg := range lpkgs { @@ -668,6 +672,11 @@ func (cfg *Cfg) readForPkgType(lpkgs []*pkg.LocalPackage, return err } } + return nil +} +func (cfg *Cfg) readValsForPkgType(lpkgs []*pkg.LocalPackage, + features map[string]bool) error { + for _, lpkg := range lpkgs { if err := cfg.readValsOnce(lpkg, features); err != nil { return err @@ -724,7 +733,19 @@ func Read(lpkgs []*pkg.LocalPackage, apis []string, pkg.PACKAGE_TYPE_APP, pkg.PACKAGE_TYPE_TARGET, } { - if err := cfg.readForPkgType(lpkgMap[ptype], features); err != nil { + if err := cfg.readDefsForPkgType(lpkgMap[ptype], features); err != nil { + return cfg, err + } + } + + for _, ptype := range []interfaces.PackageType{ + pkg.PACKAGE_TYPE_LIB, + pkg.PACKAGE_TYPE_BSP, + pkg.PACKAGE_TYPE_UNITTEST, + pkg.PACKAGE_TYPE_APP, + pkg.PACKAGE_TYPE_TARGET, + } { + if err := cfg.readValsForPkgType(lpkgMap[ptype], features); err != nil { return cfg, err } }
