Merge pull request #63 from QRPp/lib-dedupe-by-stricter-name

Bring back lib deduping/caching by name

Author: rojer

cli/manifest_parser/libs_by_loc_map.go cli/manifest_parser/libs_by_name_map.go

@@ -22,31 +22,31 @@ import (
 	"github.com/mongoose-os/mos/cli/build"
 )
 
-type libByLoc struct {
+type libByName struct {
 	Lib *build.SWModule
 	mtx sync.Mutex
 }
 
-type libByLocMap struct {
-	m   map[string]*libByLoc
+type libByNameMap struct {
+	m   map[string]*libByName
 	mtx sync.Mutex
 }
 
-func newLibByLocMap() *libByLocMap {
-	return &libByLocMap{m: map[string]*libByLoc{}}
+func newLibByNameMap() *libByNameMap {
+	return &libByNameMap{m: map[string]*libByName{}}
 }
 
-// AddOrFetchAndLock() tries to add a new location key to the set.  If
-// successful, the new entry (Lib: nil) is locked and returned; otherwise (the
-// location key already exists) the pre-existing entry is locked and returned.
-func (lm *libByLocMap) AddOrFetchAndLock(loc string) *libByLoc {
+// AddOrFetchAndLock() tries to add a new name key to the set.  If successful,
+// the new entry (Lib: nil) is locked and returned; otherwise (the name key
+// already exists) the pre-existing entry is locked and returned.
+func (lm *libByNameMap) AddOrFetchAndLock(name string) *libByName {
 	lm.mtx.Lock()
 	defer lm.mtx.Unlock()
 
-	ls, ok := lm.m[loc]
+	ls, ok := lm.m[name]
 	if !ok {
-		ls = &libByLoc{}
-		lm.m[loc] = ls
+		ls = &libByName{}
+		lm.m[name] = ls
 	}
 
 	ls.mtx.Lock()

cli/manifest_parser/manifest_parser.go

@@ -528,8 +528,8 @@ type manifestParseContext struct {
 
 	prepareLibs []*prepareLibsEntry
 
-	mtx       *sync.Mutex
-	libsByLoc *libByLocMap
+	mtx        *sync.Mutex
+	libsByName *libByNameMap
 }
 
 // readManifestWithLibs reads manifest from the provided dir, "expands" all
@@ -568,8 +568,8 @@ func readManifestWithLibs(
 
 		cbs: cbs,
 
-		mtx:       &sync.Mutex{},
-		libsByLoc: newLibByLocMap(),
+		mtx:        &sync.Mutex{},
+		libsByName: newLibByNameMap(),
 	}
 
 	manifest, mtime, err := readManifestWithLibs2(dir, pc)
@@ -909,14 +909,14 @@ func prepareLib(
 		return
 	}
 
-	ls := pc.libsByLoc.AddOrFetchAndLock(m.Location)
+	ls := pc.libsByName.AddOrFetchAndLock(m.Name)
 	defer ls.mtx.Unlock()
 	if ls.Lib != nil {
 		prepareLibReencounter(parentNodeName, manifest, pc, ls.Lib, m)
 		return
 	}
 
-	ourutil.Freportf(pc.logWriter, "Reading lib at %q...", m.Location)
+	ourutil.Freportf(pc.logWriter, "Reading lib %q at %q...", m.Name, m.Location)
 
 	libLocalDir, err := pc.cbs.ComponentProvider.GetLibLocalPath(
 		m, pc.rootAppDir, pc.appManifest.LibsVersion, manifest.Platform,
@@ -947,22 +947,73 @@ func prepareLib(
 		return
 	}
 
-	// The library can explicitly set its name in its manifest.  Also its
-	// name can be explicitly set in the referring manifest.  Either
-	// separately is fine.  None is fine too (the name defaults to the
-	// location basename).  However if both are used, treat a mismatch as an
-	// error (building without one of the two names in place is guaranteed
-	// to fail, and is likely with both too).
+	// The name of a library can be explicitly set in its manifest and/or in the
+	// referring manifest.  Barring that, the name defaults to the location
+	// basename.
+	//
+	// The library code is hardwired to the right name via at least its
+	// mgos_*_init() symbol.  The code using the library may also be via, e.g.,
+	// HAVE_* variables.
+	//
+	// The building process uses the library name for library deduplication and
+	// overriding.
+	//
+	// The location being `.../foo', the library name in use is:
+	//
+	// (#)  lib mos.yml  ref mos.yml  name in use
+	// --------------------------------------
+	// (1)  (none)       (none)       foo
+	// (2)  (none)       foo          foo
+	// (3)  (none)       bar          bar
+	// (4)  foo          (none)       foo
+	// (5)  foo          foo          foo
+	// (6)  foo          bar          (ERROR)
+	// (7)  bar          (none)       (ERROR)
+	// (8)  bar          foo          (ERROR)
+	// (9)  bar          bar          bar
+	//
+	// Rationales per (#) case:
+	// - (1) The most typical use case.
+	// - (2, 4, 5) Effectively no new information atop the location basename.
+	// - (3) The handy use case of, e.g., .../foo-test1, .../foo-test2 copies
+	//   adjacent to one another and/or the original .../foo.
+	// - (6) If `foo' were correct here, the same problem as in (7) would apply.
+	//   If `bar' were correct, then `foo' in lib mos.yml would produce erroneous
+	//   results via any automation/content generation applied to individual
+	//   libraries outside their usages.
+	// - (7) The name set via the library manifest must be replicated in the
+	//   referring manifest.  Otherwise, if this library is overridden while this
+	//   particular location isn't accessible from the building environment,
+	//   library deduplication/overriding by name can't obviate the need to read
+	//   the inaccessible manifest.
+	// - (8) This is (6) with the names swapped around.
+	// - (9) A location-independently named library.  E.g., Github repositories
+	//   .../mgos-foo, .../mgos-bar with libraries next to repositories unrelated
+	//   to Mongoose OS.
+	//
+	// At this point, libRefName == `ref mos.yml' name above; libManifest.name ==
+	// `lib mos.yml' name; m.Name == libRefName || library location basename.
+	// After validation, m.Name will be the library `name to use' above.
 	if libManifest.Name != "" {
-		if libRefName != "" && libRefName != libManifest.Name {
+		if libRefName != "" && libRefName != libManifest.Name {  // (6, 8) above
 			lpres <- libPrepareResult{
 				err: fmt.Errorf("Library %q at %q is referred to as %q from %q",
 					libManifest.Name, m.Location,
 					libRefName, manifest.Origin),
 			}
 			return
 		}
-		m.Name = libManifest.Name
+		if libRefName == "" && m.Name != libManifest.Name {  // (7) above
+			lpres <- libPrepareResult{
+				err: fmt.Errorf("Library %q at %q must be referred to as %q from %q",
+					libManifest.Name, m.Location,
+					libManifest.Name, manifest.Origin),
+			}
+			return
+		}
+	}
+	if libRefName != "" && m.Name != libRefName {  // (3, 9) above
+		m.Name = libRefName
 	}
 	name, err := m.GetName()
 	if err != nil {

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/lib1-mft-name/mos.yml cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/lib1-lib-name/mos.yml

@@ -0,0 +1,4 @@
+type: lib
+name: mylib1
+sources: [src]
+manifest_version: 2020-08-02

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/lib1-mft-name/src cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/lib1-lib-name/src

@@ -0,0 +1 @@
+../lib-blank/src

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/mylib1-lib-name/mos.yml

@@ -0,0 +1,8 @@
+manifest_version: 2020-08-02
+
+libs:
+  - location: libs/mylib1
+
+# prepareLib() commentary case (1): a library in use has no explicit `name'
+# settings.  Check that the generated MGOS_HAVE_* and mgos_*_init() identifiers
+# are proper.

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/common/mylib1-lib-name/src

@@ -0,0 +1,80 @@
+/* This file is auto-generated by mos build, do not edit! */
+
+#include <stdbool.h>
+#include <stdio.h>
+
+#include "common/cs_dbg.h"
+
+#include "mgos_app.h"
+
+extern bool mgos_core_init(void);
+extern bool mgos_mylib1_init(void);
+
+
+#ifndef MGOS_LIB_INFO_VERSION
+struct mgos_lib_info {
+  const char *name;
+  const char *version;
+  const char *repo_version;
+  const char *binary_libs;
+  bool (*init)(void);
+};
+#define MGOS_LIB_INFO_VERSION 2
+#endif
+
+#ifndef MGOS_MODULE_INFO_VERSION
+struct mgos_module_info {
+  const char *name;
+  const char *repo_version;
+};
+#define MGOS_MODULE_INFO_VERSION 1
+#endif
+
+const struct mgos_lib_info mgos_libs_info[] = {
+
+    // "core". deps: [ ]
+#if MGOS_LIB_INFO_VERSION == 1
+    {.name = "core", .version = NULL, .init = mgos_core_init},
+#else
+    {.name = "core", .version = NULL, .repo_version = NULL, .binary_libs = NULL, .init = mgos_core_init},
+#endif
+
+    // "mylib1". deps: [ "core" ]
+#if MGOS_LIB_INFO_VERSION == 1
+    {.name = "mylib1", .version = NULL, .init = mgos_mylib1_init},
+#else
+    {.name = "mylib1", .version = NULL, .repo_version = NULL, .binary_libs = NULL, .init = mgos_mylib1_init},
+#endif
+
+    // Last entry.
+    {.name = NULL},
+};
+
+const struct mgos_module_info mgos_modules_info[] = {
+
+    {.name = "mongoose-os", .repo_version = NULL},
+
+    // Last entry.
+    {.name = NULL},
+};
+
+bool mgos_deps_init(void) {
+  for (const struct mgos_lib_info *l = mgos_libs_info; l->name != NULL; l++) {
+#if MGOS_LIB_INFO_VERSION == 1
+    LOG(LL_DEBUG, ("Init %s %s...", l->name, (l->version ? l->version : "")));
+#else
+    LOG(LL_DEBUG, ("Init %s %s (%s)...",
+          l->name,
+          (l->version ? l->version : ""),
+          (l->repo_version ? l->repo_version : "")));
+#endif
+    if (l->init != NULL && !l->init()) {
+      LOG(LL_ERROR, ("%s init failed", l->name));
+      return false;
+    }
+  }
+  for (const struct mgos_module_info *m = mgos_modules_info; m->name != NULL; m++) {
+    LOG(LL_DEBUG, ("Module %s %s", m->name, (m->repo_version ? m->repo_version : "")));
+  }
+  return true;
+}

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_mft_name/app/mos.yml

@@ -0,0 +1,13 @@
+app_name: app
+libs:
+- name: core
+  location: https://github.com/mongoose-os-libs/core
+  version: "0.01"
+- name: mylib1
+  location: libs/mylib1
+  version: "0.01"
+modules:
+- name: mongoose-os
+  location: https://github.com/cesanta/mongoose-os
+  version: "0.01"
+manifest_version: "2021-03-26"

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_mft_name/libs/mylib1

@@ -0,0 +1,48 @@
+name: app
+type: app
+platform: esp32
+platforms:
+__ALL_PLATFORMS__
+sources:
+- __APP_ROOT__/libs/core/src/lib.c
+- __APP_ROOT__/libs/mylib1/src/lib.c
+- __APP_ROOT__/app/build/gen/mgos_deps_init.c
+modules:
+- name: mongoose-os
+  location: https://github.com/cesanta/mongoose-os
+  version: "0.01"
+build_vars:
+  BOARD: ""
+  ESP_IDF_EXTRA_COMPONENTS: ""
+  ESP_IDF_SDKCONFIG_OPTS: ""
+  MGOS: "1"
+  MGOS_HAVE_CORE: "1"
+  MGOS_HAVE_MYLIB1: "1"
+cdefs:
+  MGOS: "1"
+  MGOS_HAVE_CORE: "1"
+  MGOS_HAVE_MYLIB1: "1"
+libs_version: "0.01"
+modules_version: "0.01"
+mongoose_os_version: "0.01"
+manifest_version: "2020-08-02"
+libs_handled:
+- lib:
+    name: core
+    location: https://github.com/mongoose-os-libs/core
+  path: __APP_ROOT__/libs/core
+  sources:
+  - __APP_ROOT__/libs/core/src/lib.c
+  version: "0.01"
+- lib:
+    name: mylib1
+    location: libs/mylib1
+  path: __APP_ROOT__/libs/mylib1
+  init_deps:
+  - core
+  sources:
+  - __APP_ROOT__/libs/mylib1/src/lib.c
+  version: "0.01"
+init_deps:
+- core
+- mylib1

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_no_name/app/mos.yml

@@ -0,0 +1,9 @@
+manifest_version: 2020-08-02
+
+libs:
+  - location: libs/mylib1
+    name: mylib1
+
+# prepareLib() commentary case (2): a library in use has `name' set in referring
+# manifest that corresponds to the basename of its location.  Check that the
+# generated MGOS_HAVE_* and mgos_*_init() identifiers are unaffected.

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_no_name/expected/esp32/mgos_deps_init.c

@@ -0,0 +1 @@
+../test_01_no_name/expected

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_no_name/expected/esp32/mgos_deps_manifest.yml

@@ -0,0 +1 @@
+../test_01_no_name/libs

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_no_name/expected/esp32/mos_final.yml

@@ -0,0 +1,9 @@
+manifest_version: 2020-08-02
+
+libs:
+  - location: libs/mylib1
+    name: lib1
+
+# prepareLib() commentary case (3): a library in use has `name' set in referring
+# manifest that is distinct from its location basename.  Check that the
+# generated MGOS_HAVE_* and mgos_*_init() identifiers are properly altered.

cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_mft_name/libs/core cli/manifest_parser/test_manifests/testset_10_lib_name_settings/test_01_no_name/libs/core

@@ -0,0 +1 @@
+../test_01_no_name/libs