From c4666109c9d833ff2ca84cf69f8a4f3781bb25f1 Mon Sep 17 00:00:00 2001 From: Charles Oliveira Date: Thu, 17 Mar 2022 21:51:22 -0300 Subject: [PATCH] installer: normalize paths By default, CMSIS_PACK_ROOT environment variable is defined in a Windows environment as C:\SOME_PATH. Both bash and cmd work fine with that approach. In git-bash/mingw, if the user tries to "export CMSIS_PACK_ROOT=C:\SOME_PATH" or "--pack-root/-R C:\SOME_PATH", bash automatically removes the backwards slash "\" because it is considered to be a escape character, so it becomes "C:SOME_PATH", which is a valid file name. It's wrong, but valid. I wrongly assumed that this would throw an error. The colon (":") character is not allowed in directory names on Windows (https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#naming-conventions). But in really, that does not fail in neither bash nor cmd: * in bash, this becomes a valid name "C:SOME_PATH" which Windows later translates it to the unicode character "61498": https://wutils.com/unicode/61498 which is valid. * in cmd, this becomes "SOME_PATH" only, which is still valid. To fix that, users should either escape all backwards slashes "C:\\SOME_PATH\\SUB_DIR" or wrap it around single/double quotes `cpackget -R "C:\SOME_PATH"`. Another finding is that bash paths formatted as "/c/SOME_PATH" are always translated to "C:/SOME_PATH" with the forward slash. And this is where cpackget gets confused, because when it lists installed packs, it all start as "C:\SOME_PATH". This means that cpackget will never receive paths in "/c/" format. It seems this is just a visualization feature of bash. By using the correct approach to set the path, cpackget works just fine. Signed-off-by: Charles Oliveira --- cmd/errors/errors.go | 1 + cmd/installer/root.go | 11 ++++++++-- cmd/installer/root_test.go | 35 ++++++++++++++++++++++++++---- cmd/installer/root_unix_test.go | 31 ++++++++++++++++++++++++++ cmd/installer/root_windows_test.go | 34 +++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 6 deletions(-) create mode 100644 cmd/installer/root_unix_test.go create mode 100644 cmd/installer/root_windows_test.go diff --git a/cmd/errors/errors.go b/cmd/errors/errors.go index e386f82..8ba249f 100644 --- a/cmd/errors/errors.go +++ b/cmd/errors/errors.go @@ -36,6 +36,7 @@ var ( ErrExtractEula = errors.New("user wants to extract embedded license only") ErrLicenseNotFound = errors.New("embedded license not found") ErrPackRootNotFound = errors.New("no CMSIS Pack Root directory specified. Either the environment CMSIS_PACK_ROOT needs to be set or the path specified using the command line option -R/--pack-root string") + ErrPackRootDoesNotExist = errors.New("the specified CMSIS Pack Root directory does NOT exist! Please take a moment to review if the value is correct or create a new one via `cpackget init` command") ErrPdscFileTooDeepInPack = errors.New("pdsc file is too deep in pack file") // Errors related to network diff --git a/cmd/installer/root.go b/cmd/installer/root.go index 6ceab4c..0db2831 100644 --- a/cmd/installer/root.go +++ b/cmd/installer/root.go @@ -247,6 +247,7 @@ func ListInstalledPacks(listCached, listPublic bool) error { return strings.ToLower(matches[i]) < strings.ToLower(matches[j]) }) for _, pdscFilePath := range matches { + log.Debug(pdscFilePath) // Transform pdscFilePath into packName pdscFilePath = strings.Replace(pdscFilePath, Installation.PackRoot, "", -1) @@ -344,10 +345,16 @@ var Installation *PacksInstallationType // SetPackRoot sets the working directory of the packs installation // if create == true, cpackget will try to create needed resources func SetPackRoot(packRoot string, create bool) error { + if len(packRoot) == 0 { + log.Infof("Using pack root: \"%v\"", packRoot) + return errs.ErrPackRootNotFound + } + + packRoot = filepath.Clean(packRoot) log.Infof("Using pack root: \"%v\"", packRoot) - if len(packRoot) == 0 || (!utils.DirExists(packRoot) && !create) { - return errs.ErrPackRootNotFound + if !utils.DirExists(packRoot) && !create { + return errs.ErrPackRootDoesNotExist } Installation = &PacksInstallationType{ diff --git a/cmd/installer/root_test.go b/cmd/installer/root_test.go index ffbfa06..b537c57 100644 --- a/cmd/installer/root_test.go +++ b/cmd/installer/root_test.go @@ -435,10 +435,20 @@ func TestUpdatePublicIndex(t *testing.T) { }) } +func checkPackRoot(t *testing.T, path string) { + assert := assert.New(t) + + assert.True(utils.DirExists(path)) + assert.True(utils.DirExists(installer.Installation.DownloadDir)) + assert.True(utils.DirExists(installer.Installation.WebDir)) + assert.True(utils.DirExists(installer.Installation.LocalDir)) +} + func TestSetPackRoot(t *testing.T) { assert := assert.New(t) + // Sanity tests t.Run("test fail to initialize empty pack root", func(t *testing.T) { localTestingDir := "" err := installer.SetPackRoot(localTestingDir, !CreatePackRoot) @@ -448,19 +458,36 @@ func TestSetPackRoot(t *testing.T) { assert.Equal(errs.ErrPackRootNotFound, err) }) + t.Run("test fail to use non-existing directory", func(t *testing.T) { + localTestingDir := "non-existing-dir" + err := installer.SetPackRoot(localTestingDir, !CreatePackRoot) + assert.Equal(errs.ErrPackRootDoesNotExist, err) + }) + t.Run("test initialize pack root", func(t *testing.T) { localTestingDir := "valid-pack-root" defer os.RemoveAll(localTestingDir) assert.Nil(installer.SetPackRoot(localTestingDir, CreatePackRoot)) - assert.True(utils.DirExists(localTestingDir)) - assert.True(utils.DirExists(installer.Installation.DownloadDir)) - assert.True(utils.DirExists(installer.Installation.WebDir)) - assert.True(utils.DirExists(installer.Installation.LocalDir)) + checkPackRoot(t, localTestingDir) // Now just make sure it's usable, even when not forced to initialize assert.Nil(installer.SetPackRoot(localTestingDir, !CreatePackRoot)) }) + + // Define a few paths to try out per operating system + paths := generatePaths(t) + for description, path := range paths { + t.Run("test "+description, func(t *testing.T) { + defer os.RemoveAll(path) + assert.Nil(installer.SetPackRoot(path, CreatePackRoot)) + + checkPackRoot(t, path) + + // Now just make sure it's usable, even when not forced to initialize + assert.Nil(installer.SetPackRoot(path, !CreatePackRoot)) + }) + } } func init() { diff --git a/cmd/installer/root_unix_test.go b/cmd/installer/root_unix_test.go new file mode 100644 index 0000000..a27356b --- /dev/null +++ b/cmd/installer/root_unix_test.go @@ -0,0 +1,31 @@ +//go:build !windows +// +build !windows + +/* SPDX-License-Identifier: Apache-2.0 */ +/* Copyright Contributors to the cpackget project. */ + +package installer_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" +) + +func generatePaths(t *testing.T) map[string]string { + cwd, err := os.Getwd() + assert.Nil(t, err) + + dirName := "valid-pack-root" + absPath := filepath.Join(cwd, dirName) + + // Define a few paths to try out + return map[string]string{ + "regular absolute path": absPath, + "absolute path with ..": filepath.Join(absPath, "..", dirName), + "multiple leading slashes": "////" + absPath, + "multiple trailing slashes": absPath + "////", + } +} diff --git a/cmd/installer/root_windows_test.go b/cmd/installer/root_windows_test.go new file mode 100644 index 0000000..83531b2 --- /dev/null +++ b/cmd/installer/root_windows_test.go @@ -0,0 +1,34 @@ +//go:build windows +// +build windows + +/* SPDX-License-Identifier: Apache-2.0 */ +/* Copyright Contributors to the cpackget project. */ + +package installer_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func generatePaths(t *testing.T) map[string]string { + cwd, err := os.Getwd() + assert.Nil(t, err) + + dirName := "valid-pack-root" + absPath := filepath.Join(cwd, dirName) + + // Define a few paths to try out + return map[string]string{ + "regular absolute path": absPath, + "absolute path with ..": filepath.Join(absPath, "..", dirName), + "absolute path with :/": strings.Replace(absPath, ":\\", "/", 1), + "all forward slashes": strings.ReplaceAll(absPath, "\\", "/"), + "multiple leading slashes": strings.Replace(absPath, ":\\", ":\\\\\\\\", 1), + "multiple trailing slashes": absPath + "\\\\\\\\", + } +}