From cb7b0fb1892b6895e39aeabaa798e3751976e0c9 Mon Sep 17 00:00:00 2001 From: per1234 Date: Sun, 2 Oct 2022 00:16:50 -0700 Subject: [PATCH] [skip changelog] Use appropriate name for field that stores library folder name Each Arduino library has a name, which is used as its sole identifier by the user in `arduino-cli lib` commands, and by Arduino CLI when referring to the library in messages displayed to the user. The name is defined by: - "1.5 format" libraries: `name` field in the library.properties metadata file - "1.0 format" (AKA "legacy") libraries: installation folder name The name is resolved when loading the library and stored in the `Name` field of the `github.com/arduino/arduino-cli/arduino/libraries.Library` struct. The name of the library's installation folder is used by Arduino CLI in several other ways, most notably for determining "folder name priority" for use in library dependency resolution. For this reason, the folder name is also stored in the struct when loading the library. Arduino CLI and arduino-builder have been plagued by problems caused by the inappropriate use of this folder name as the identifier for the library instead of the sole correct identifier (which is only the folder name in the case of "1.0 format" libraries. The design of the `github.com/arduino/arduino-cli/arduino/libraries.Library` struct may have been a contributing factor in those bugs, since at the time of their occurrence the folder name was stored in the `Name` field, the metadata-defined name in a `RealName`. In addition to the fact that no one field could be used as a source of the name in all cases, I suspect the ambiguous field names themselves caused confusion to developers. This situation was improved by providing the library identifier via a single field for all library formats. The name provided by this field is the "canonical" name of the library. Inexplicably, at that time the field containing the folder name was renamed "CanonicalName". The string contained by this field is in no way a "canonical" name for the library, so the field name is bound to cause more of the same bugs and confusion the redesign of the struct was intended to prevent. The inappropriately named `github.com/arduino/arduino-cli/arduino/libraries.Library.CanonicalName` field is hereby renamed to the accurate `DirName`. --- arduino/libraries/libraries.go | 2 +- arduino/libraries/librariesresolver/cpp.go | 14 +++++++------- arduino/libraries/librariesresolver/cpp_test.go | 12 ++++++------ arduino/libraries/loader.go | 4 ++-- docs/UPGRADING.md | 10 +++++----- internal/integrationtest/lib/lib_test.go | 2 +- legacy/builder/create_cmake_rule.go | 2 +- legacy/builder/phases/libraries_builder.go | 4 ++-- legacy/builder/types/types.go | 2 +- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/arduino/libraries/libraries.go b/arduino/libraries/libraries.go index ab0d2a380f7..551705afb99 100644 --- a/arduino/libraries/libraries.go +++ b/arduino/libraries/libraries.go @@ -63,7 +63,7 @@ type Library struct { Types []string `json:"types,omitempty"` InstallDir *paths.Path - CanonicalName string + DirName string SourceDir *paths.Path UtilityDir *paths.Path Location LibraryLocation diff --git a/arduino/libraries/librariesresolver/cpp.go b/arduino/libraries/librariesresolver/cpp.go index 0078fdcca78..2f256b2218f 100644 --- a/arduino/libraries/librariesresolver/cpp.go +++ b/arduino/libraries/librariesresolver/cpp.go @@ -168,7 +168,7 @@ func computePriority(lib *libraries.Library, header, arch string) int { header = strings.TrimSuffix(header, filepath.Ext(header)) header = simplify(header) name := simplify(lib.Name) - canonicalName := simplify(lib.CanonicalName) + dirName := simplify(lib.DirName) priority := 0 @@ -185,17 +185,17 @@ func computePriority(lib *libraries.Library, header, arch string) int { priority += 0 } - if name == header && canonicalName == header { + if name == header && dirName == header { priority += 600 - } else if name == header || canonicalName == header { + } else if name == header || dirName == header { priority += 500 - } else if name == header+"-master" || canonicalName == header+"-master" { + } else if name == header+"-master" || dirName == header+"-master" { priority += 400 - } else if strings.HasPrefix(name, header) || strings.HasPrefix(canonicalName, header) { + } else if strings.HasPrefix(name, header) || strings.HasPrefix(dirName, header) { priority += 300 - } else if strings.HasSuffix(name, header) || strings.HasSuffix(canonicalName, header) { + } else if strings.HasSuffix(name, header) || strings.HasSuffix(dirName, header) { priority += 200 - } else if strings.Contains(name, header) || strings.Contains(canonicalName, header) { + } else if strings.Contains(name, header) || strings.Contains(dirName, header) { priority += 100 } diff --git a/arduino/libraries/librariesresolver/cpp_test.go b/arduino/libraries/librariesresolver/cpp_test.go index 6f8b195be61..d378a990bb8 100644 --- a/arduino/libraries/librariesresolver/cpp_test.go +++ b/arduino/libraries/librariesresolver/cpp_test.go @@ -147,14 +147,14 @@ func TestCppHeaderResolver(t *testing.T) { func TestCppHeaderResolverWithLibrariesInStrangeDirectoryNames(t *testing.T) { resolver := NewCppResolver() librarylist := libraries.List{} - librarylist.Add(&libraries.Library{CanonicalName: "onewire_2_3_4", Name: "OneWire", Architectures: []string{"*"}}) - librarylist.Add(&libraries.Library{CanonicalName: "onewireng_2_3_4", Name: "OneWireNg", Architectures: []string{"avr"}}) + librarylist.Add(&libraries.Library{DirName: "onewire_2_3_4", Name: "OneWire", Architectures: []string{"*"}}) + librarylist.Add(&libraries.Library{DirName: "onewireng_2_3_4", Name: "OneWireNg", Architectures: []string{"avr"}}) resolver.headers["OneWire.h"] = librarylist - require.Equal(t, "onewire_2_3_4", resolver.ResolveFor("OneWire.h", "avr").CanonicalName) + require.Equal(t, "onewire_2_3_4", resolver.ResolveFor("OneWire.h", "avr").DirName) librarylist2 := libraries.List{} - librarylist2.Add(&libraries.Library{CanonicalName: "OneWire", Name: "OneWire", Architectures: []string{"*"}}) - librarylist2.Add(&libraries.Library{CanonicalName: "onewire_2_3_4", Name: "OneWire", Architectures: []string{"avr"}}) + librarylist2.Add(&libraries.Library{DirName: "OneWire", Name: "OneWire", Architectures: []string{"*"}}) + librarylist2.Add(&libraries.Library{DirName: "onewire_2_3_4", Name: "OneWire", Architectures: []string{"avr"}}) resolver.headers["OneWire.h"] = librarylist2 - require.Equal(t, "OneWire", resolver.ResolveFor("OneWire.h", "avr").CanonicalName) + require.Equal(t, "OneWire", resolver.ResolveFor("OneWire.h", "avr").DirName) } diff --git a/arduino/libraries/loader.go b/arduino/libraries/loader.go index f6cd0b679bb..86309d7252b 100644 --- a/arduino/libraries/loader.go +++ b/arduino/libraries/loader.go @@ -108,7 +108,7 @@ func makeNewLibrary(libraryDir *paths.Path, location LibraryLocation) (*Library, if err := addExamples(library); err != nil { return nil, errors.Errorf(tr("scanning examples: %s"), err) } - library.CanonicalName = libraryDir.Base() + library.DirName = libraryDir.Base() library.Name = strings.TrimSpace(libProperties.Get("name")) library.Author = strings.TrimSpace(libProperties.Get("author")) library.Maintainer = strings.TrimSpace(libProperties.Get("maintainer")) @@ -132,7 +132,7 @@ func makeLegacyLibrary(path *paths.Path, location LibraryLocation) (*Library, er SourceDir: path, Layout: FlatLayout, Name: path.Base(), - CanonicalName: path.Base(), + DirName: path.Base(), Architectures: []string{"*"}, IsLegacy: true, Version: semver.MustParse(""), diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 0cd1eb0d272..bb2af343fc9 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -9,12 +9,12 @@ Here you can find a list of migration guides to handle breaking changes between In the structure `github.com/arduino/arduino-cli/arduino/libraries.Library` the field: - `RealName` has been renamed to `Name` -- `Name` has been renamed to `CanonicalName` +- `Name` has been renamed to `DirName` -Now `Name` is the name of the library as it appears in the `library.properties` file and `CanonicalName` it's the name -of the directory containing the library. The `CanonicalName` is usually the name of the library with non-alphanumeric -characters converted to underscore, but it could be actually anything since the directory where the library is installed -can be freely renamed. +Now `Name` is the name of the library as it appears in the `library.properties` file and `DirName` it's the name of the +directory containing the library. The `DirName` is usually the name of the library with non-alphanumeric characters +converted to underscore, but it could be actually anything since the directory where the library is installed can be +freely renamed. This change improves the overall code base naming coherence since all the structures involving libraries have the `Name` field that refers to the library name as it appears in the `library.properties` file. diff --git a/internal/integrationtest/lib/lib_test.go b/internal/integrationtest/lib/lib_test.go index 661e3d8b5eb..0300de3c2ea 100644 --- a/internal/integrationtest/lib/lib_test.go +++ b/internal/integrationtest/lib/lib_test.go @@ -65,7 +65,7 @@ func TestLibUpgradeCommand(t *testing.T) { requirejson.Query(t, stdOut, `.[].library | select(.name=="Servo") | .version`, servoVersion) } -func TestLibCommandsUsingNameInsteadOfCanonicalName(t *testing.T) { +func TestLibCommandsUsingNameInsteadOfDirName(t *testing.T) { env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t) defer env.CleanUp() diff --git a/legacy/builder/create_cmake_rule.go b/legacy/builder/create_cmake_rule.go index 0bb5d9e691c..06f29d02f26 100644 --- a/legacy/builder/create_cmake_rule.go +++ b/legacy/builder/create_cmake_rule.go @@ -73,7 +73,7 @@ func (s *ExportProjectCMake) Run(ctx *types.Context) error { dynamicLibsFromPkgConfig := map[string]bool{} for _, library := range ctx.ImportedLibraries { // Copy used libraries in the correct folder - libDir := libBaseFolder.Join(library.CanonicalName) + libDir := libBaseFolder.Join(library.DirName) mcu := ctx.BuildProperties.Get(constants.BUILD_PROPERTIES_BUILD_MCU) utils.CopyDir(library.InstallDir.String(), libDir.String(), validExportExtensions) diff --git a/legacy/builder/phases/libraries_builder.go b/legacy/builder/phases/libraries_builder.go index ae1a09db11a..32bb0b2f01c 100644 --- a/legacy/builder/phases/libraries_builder.go +++ b/legacy/builder/phases/libraries_builder.go @@ -131,7 +131,7 @@ func compileLibrary(ctx *types.Context, library *libraries.Library, buildPath *p if ctx.Verbose { ctx.Info(tr(`Compiling library "%[1]s"`, library.Name)) } - libraryBuildPath := buildPath.Join(library.CanonicalName) + libraryBuildPath := buildPath.Join(library.DirName) if err := libraryBuildPath.MkdirAll(); err != nil { return nil, errors.WithStack(err) @@ -189,7 +189,7 @@ func compileLibrary(ctx *types.Context, library *libraries.Library, buildPath *p return nil, errors.WithStack(err) } if library.DotALinkage { - archiveFile, err := builder_utils.ArchiveCompiledFiles(ctx, libraryBuildPath, paths.New(library.CanonicalName+".a"), libObjectFiles, buildProperties) + archiveFile, err := builder_utils.ArchiveCompiledFiles(ctx, libraryBuildPath, paths.New(library.DirName+".a"), libObjectFiles, buildProperties) if err != nil { return nil, errors.WithStack(err) } diff --git a/legacy/builder/types/types.go b/legacy/builder/types/types.go index ff12a63eafd..855c0295ffb 100644 --- a/legacy/builder/types/types.go +++ b/legacy/builder/types/types.go @@ -53,7 +53,7 @@ func buildRoot(ctx *Context, origin interface{}) *paths.Path { case *sketch.Sketch: return ctx.SketchBuildPath case *libraries.Library: - return ctx.LibrariesBuildPath.Join(o.CanonicalName) + return ctx.LibrariesBuildPath.Join(o.DirName) default: panic("Unexpected origin for SourceFile: " + fmt.Sprint(origin)) }