From dd48c9e6fb06e4c02dd4d13504f3463323e5ce86 Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 5 Jul 2024 13:06:12 +0200 Subject: [PATCH 1/3] Don't unecessarily change the package.lock and package.yaml files --- Makefile | 3 +- pkg/tpkg/lock_file.go | 55 ++++++++++++++++++- pkg/tpkg/spec.go | 31 ++++++++++- pkg/tpkg/ui.go | 13 +++++ .../pkg/InstallNoMTimeChange/gold/test.gold | 9 +++ .../assets/pkg/InstallNoMTimeChange/main.toit | 6 ++ .../pkg/InstallNoMTimeChange/package.lock | 7 +++ tests/pkg_test.go | 54 ++++++++++++------ 8 files changed, 155 insertions(+), 23 deletions(-) create mode 100644 tests/assets/pkg/InstallNoMTimeChange/gold/test.gold create mode 100644 tests/assets/pkg/InstallNoMTimeChange/main.toit create mode 100644 tests/assets/pkg/InstallNoMTimeChange/package.lock diff --git a/Makefile b/Makefile index 4a62efb..d9cfe11 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,8 @@ BUILD_DIR := build GO_PACKAGE := github.com/toitlang/tpkg -TOITC_PATH ?= toit.compile +# The TOITC_PATH needs to be absolute so that 'toit.lsp' can find it. +TOITC_PATH ?= $(shell command -v toit.compile) TOITLSP_PATH ?= toit.lsp TOITVM_PATH ?= toit.run TOITPKG_PATH ?= $(CURDIR)/$(BUILD_DIR)/toitpkg diff --git a/pkg/tpkg/lock_file.go b/pkg/tpkg/lock_file.go index ecb0074..a32ba89 100644 --- a/pkg/tpkg/lock_file.go +++ b/pkg/tpkg/lock_file.go @@ -17,7 +17,7 @@ package tpkg import ( "fmt" - "io/ioutil" + "os" "path/filepath" "strings" @@ -59,6 +59,44 @@ type PackageEntry struct { Prefixes PrefixMap `yaml:"prefixes,omitempty"` } +func (lf *LockFile) equals(other *LockFile) bool { + if lf.SDK != other.SDK { + return false + } + if !prefixMapEquals(lf.Prefixes, other.Prefixes) { + return false + } + if len(lf.Packages) != len(other.Packages) { + return false + } + for k, v := range lf.Packages { + otherV, ok := other.Packages[k] + if !ok || !v.equals(otherV) { + return false + } + } + return true +} + +func (pe PackageEntry) equals(other PackageEntry) bool { + if pe.URL != other.URL || pe.Name != other.Name || pe.Version != other.Version || pe.Path != other.Path || pe.Hash != other.Hash { + return false + } + return prefixMapEquals(pe.Prefixes, other.Prefixes) +} + +func prefixMapEquals(a, b PrefixMap) bool { + if len(a) != len(b) { + return false + } + for k, v := range a { + if v != b[k] { + return false + } + } + return true +} + // PrefixMap has a mapping from prefix to package-id. type PrefixMap map[string]string @@ -76,7 +114,7 @@ func (pe PackageEntry) Validate(ui UI) error { // ReadLockFile reads the lock-file at the given path. func ReadLockFile(path string) (*LockFile, error) { // TODO(florian): should we validate the file here? - b, err := ioutil.ReadFile(path) + b, err := os.ReadFile(path) if err != nil { return nil, err } @@ -91,11 +129,22 @@ func ReadLockFile(path string) (*LockFile, error) { } func (lf *LockFile) WriteToFile() error { + // Check whether the file already exists and has the same content. + // We don't want to touch files if they don't change. + + if _, err := os.Stat(lf.path); err == nil { + // File exists. Check whether it has the same content. + existingLock, err := ReadLockFile(lf.path) + if err == nil && existingLock != nil && lf.equals(existingLock) { + return nil + } + } + b, err := yaml.Marshal(lf) if err != nil { return err } - return ioutil.WriteFile(lf.path, b, 0644) + return os.WriteFile(lf.path, b, 0644) } // PrintLockFile prints the contents of the lock file for the current project. diff --git a/pkg/tpkg/spec.go b/pkg/tpkg/spec.go index 3f0b531..1d834b7 100644 --- a/pkg/tpkg/spec.go +++ b/pkg/tpkg/spec.go @@ -18,7 +18,6 @@ package tpkg import ( "fmt" "io" - "io/ioutil" "os" "path/filepath" "regexp" @@ -46,6 +45,25 @@ type SpecEnvironment struct { SDK string `yaml:"sdk,omitempty"` } +func (s *Spec) equals(other *Spec) bool { + if s.Name != other.Name || s.Description != other.Description || s.License != other.License || s.Environment.SDK != other.Environment.SDK { + return false + } + if len(s.Deps) != len(other.Deps) { + return false + } + for prefix, dep := range s.Deps { + otherDep, ok := other.Deps[prefix] + if !ok { + return false + } + if dep.URL != otherDep.URL || dep.Version != otherDep.Version || dep.Path != otherDep.Path { + return false + } + } + return true +} + // DependencyMap is a map from prefix to package. type DependencyMap map[string]SpecPackage @@ -111,7 +129,7 @@ func (s *Spec) Validate(ui UI) error { func (s *Spec) ParseFile(filename string, ui UI) error { s.path = filename - b, err := ioutil.ReadFile(filename) + b, err := os.ReadFile(filename) if err != nil { return err } @@ -134,6 +152,15 @@ func (s *Spec) WriteYAML(writer io.Writer) error { } func (s *Spec) WriteToFile() error { + // Check whether the file already exists and has the same content. + // We don't want to touch files if they don't change. + if _, err := os.Stat(s.path); err == nil { + existingSpec, err := ReadSpec(s.path, nullUI{}) + if err == nil && existingSpec != nil && s.equals(existingSpec) { + return nil + } + } + file, err := os.Create(s.path) if err != nil { return err diff --git a/pkg/tpkg/ui.go b/pkg/tpkg/ui.go index ad6e3db..8a4fc68 100644 --- a/pkg/tpkg/ui.go +++ b/pkg/tpkg/ui.go @@ -59,6 +59,19 @@ func (ui fmtUI) ReportInfo(format string, a ...interface{}) { fmt.Printf("Info: "+format+"\n", a...) } +// nullUI implements a UI that does nothing. +type nullUI struct{} + +func (ui nullUI) ReportError(format string, a ...interface{}) error { + return ErrAlreadyReported +} + +func (ui nullUI) ReportWarning(format string, a ...interface{}) { +} + +func (ui nullUI) ReportInfo(format string, a ...interface{}) { +} + var ( // ErrAlreadyReported can be used to signal that an error has // been reported, and that no further action needs to be taken. diff --git a/tests/assets/pkg/InstallNoMTimeChange/gold/test.gold b/tests/assets/pkg/InstallNoMTimeChange/gold/test.gold new file mode 100644 index 0000000..d3a636f --- /dev/null +++ b/tests/assets/pkg/InstallNoMTimeChange/gold/test.gold @@ -0,0 +1,9 @@ +// Add registry so we can find packages. +=================== +pkg registry add --local test-reg /registry_git_pkgs +Exit Code: 0 +=================== +// Just 'install' doesn't add the missing dependencies. +=================== +pkg install +Exit Code: 0 diff --git a/tests/assets/pkg/InstallNoMTimeChange/main.toit b/tests/assets/pkg/InstallNoMTimeChange/main.toit new file mode 100644 index 0000000..9c2b135 --- /dev/null +++ b/tests/assets/pkg/InstallNoMTimeChange/main.toit @@ -0,0 +1,6 @@ +// Copyright (C) 2024 Toitware ApS. All rights reserved. + +import pkg1 + +main: + print pkg1.identify diff --git a/tests/assets/pkg/InstallNoMTimeChange/package.lock b/tests/assets/pkg/InstallNoMTimeChange/package.lock new file mode 100644 index 0000000..2d8c14d --- /dev/null +++ b/tests/assets/pkg/InstallNoMTimeChange/package.lock @@ -0,0 +1,7 @@ +prefixes: + pkg1: pkg1 + +packages: + pkg1: + url: '<[*TEST_GIT_DIR_ESCAPE*]>/git_pkgs/pkg1' + version: 1.0.0 diff --git a/tests/pkg_test.go b/tests/pkg_test.go index a2997fe..7cfe80a 100644 --- a/tests/pkg_test.go +++ b/tests/pkg_test.go @@ -290,7 +290,7 @@ func copyRec(t *tedi.T, testDir string, sourceDir string, targetDir string) { } return unzip(p, filepath.Dir(target)) } - data, err := ioutil.ReadFile(p) + data, err := os.ReadFile(p) if err != nil { return err } @@ -300,7 +300,7 @@ func copyRec(t *tedi.T, testDir string, sourceDir string, targetDir string) { data = bytes.ReplaceAll(data, []byte(testDirGitPattern), []byte(testDirGitURL)) escapedTestDirGitURL := string(compiler.ToURIPath(testDirGitURL)) data = bytes.ReplaceAll(data, []byte(testDirGitEscapePattern), []byte(escapedTestDirGitURL)) - return ioutil.WriteFile(target, data, info.Mode().Perm()) + return os.WriteFile(target, data, info.Mode().Perm()) }) require.NoError(t, err) } @@ -515,7 +515,7 @@ func (pt PkgTest) updateGold(name string, newGold string) { } require.NoError(pt.t, err) goldPath := filepath.Join(goldDir, name+".gold") - oldBytes, err := ioutil.ReadFile(goldPath) + oldBytes, err := os.ReadFile(goldPath) if err == nil { oldGold := string(oldBytes) if string(oldBytes) != newGold { @@ -524,7 +524,7 @@ func (pt PkgTest) updateGold(name string, newGold string) { } else if os.IsNotExist(err) { fmt.Printf("Creating new gold %s with content:\n%s", goldPath, newGold) } - err = ioutil.WriteFile(goldPath, []byte(newGold), 0644) + err = os.WriteFile(goldPath, []byte(newGold), 0644) require.NoError(pt.t, err) } @@ -534,7 +534,7 @@ func (pt PkgTest) checkGold(name string, actual string) { return } goldPath := filepath.Join(pt.dir, "gold", name+".gold") - contentBytes, err := ioutil.ReadFile(goldPath) + contentBytes, err := os.ReadFile(goldPath) require.NoError(pt.t, err) gold := string(contentBytes) // On windows the gold files come with '\r\n'... @@ -648,12 +648,12 @@ func test_toitPkg(t *tedi.T) { Branch: plumbing.NewTagReferenceName("1.0.0"), }) require.NoError(t, err) - data, err := ioutil.ReadFile(filepath.Join(gitDir, "a")) + data, err := os.ReadFile(filepath.Join(gitDir, "a")) require.NoError(t, err) dataStr := strings.ReplaceAll(string(data), "\r\n", "\n") // Notice that we implicitly check the correct `<[*TEST_DIR*>]` replacement. assert.Equal(t, dirInFiles+"/a 1.0.0\n", dataStr) - data, err = ioutil.ReadFile(filepath.Join(gitDir, "b")) + data, err = os.ReadFile(filepath.Join(gitDir, "b")) require.NoError(t, err) dataStr = strings.ReplaceAll(string(data), "\r\n", "\n") assert.Equal(t, dirInFiles+"/b 1.0.0\n", dataStr) @@ -666,13 +666,13 @@ func test_toitPkg(t *tedi.T) { Branch: plumbing.NewTagReferenceName("2.0.0"), }) require.NoError(t, err) - data, err = ioutil.ReadFile(filepath.Join(gitDir, "a")) + data, err = os.ReadFile(filepath.Join(gitDir, "a")) require.NoError(t, err) dataStr = strings.ReplaceAll(string(data), "\r\n", "\n") assert.Equal(t, dirInFiles+"/a 2.0.0\n", dataStr) _, err = os.Stat(filepath.Join(gitDir, "b")) assert.True(t, os.IsNotExist(err)) - data, err = ioutil.ReadFile(filepath.Join(gitDir, "c")) + data, err = os.ReadFile(filepath.Join(gitDir, "c")) require.NoError(t, err) dataStr = strings.ReplaceAll(string(data), "\r\n", "\n") assert.Equal(t, dirInFiles+"/c 2.0.0\n", dataStr) @@ -1130,10 +1130,10 @@ func test_toitPkg(t *tedi.T) { {"pkg", "list"}, }) - data, err := ioutil.ReadFile(filepath.Join(pt.dir, yamlFile)) + data, err := os.ReadFile(filepath.Join(pt.dir, yamlFile)) require.NoError(t, err) pkgTestSpecPath := filepath.Join(regPath, yamlFile) - err = ioutil.WriteFile(pkgTestSpecPath, data, 0644) + err = os.WriteFile(pkgTestSpecPath, data, 0644) require.NoError(t, err) repository, err := git.PlainOpen(regPath) @@ -1179,7 +1179,7 @@ func test_toitPkg(t *tedi.T) { // Touch a file in the cache path, so we can see that the clear-cache // actually removes it. cacheFile := filepath.Join(regCachePath, "foo") - err := ioutil.WriteFile(cacheFile, []byte("foo"), 0644) + err := os.WriteFile(cacheFile, []byte("foo"), 0644) require.NoError(t, err) if i == 0 { @@ -1359,7 +1359,7 @@ func test_toitPkg(t *tedi.T) { }) yamlPath := filepath.Join(pt.dir, "package.yaml") - err := ioutil.WriteFile(yamlPath, []byte{}, 0644) + err := os.WriteFile(yamlPath, []byte{}, 0644) assert.NoError(t, err) pt.GoldToit("test2", [][]string{ @@ -1378,7 +1378,7 @@ func test_toitPkg(t *tedi.T) { {"pkg", "registry", "add", "toit", "github.com/toitware/registry"}, }) configPath := filepath.Join(pt.dir, "config.yaml") - data, err := ioutil.ReadFile(configPath) + data, err := os.ReadFile(configPath) assert.NoError(t, err) assert.Contains(t, string(data), "toitware/registry") }) @@ -1473,7 +1473,7 @@ func test_toitPkg(t *tedi.T) { }) descPath := filepath.Join(pt.dir, "out", "packages", "github.com", "toitware", "toit-morse", "1.0.6", "desc.yaml") assert.FileExists(t, descPath) - _, err := ioutil.ReadFile(descPath) + _, err := os.ReadFile(descPath) assert.NoError(t, err) }) @@ -1647,13 +1647,13 @@ func test_toitPkg(t *tedi.T) { // Modifying the version constraint in the package.spec is copied to the // lock file. packageSpecPath := filepath.Join(pt.dir, "package.yaml") - data, err := ioutil.ReadFile(packageSpecPath) + data, err := os.ReadFile(packageSpecPath) assert.NoError(t, err) str := string(data) + ` environment: sdk: ^1.20.0 ` - err = ioutil.WriteFile(packageSpecPath, []byte(str), 0644) + err = os.WriteFile(packageSpecPath, []byte(str), 0644) assert.NoError(t, err) pt.GoldToit("test3", [][]string{ {"// sdkVersion = "}, @@ -1709,4 +1709,24 @@ environment: } wg.Wait() }) + + t.Run("InstallNoMTimeChange", func(t *tedi.T, pt PkgTest) { + regPath := filepath.Join(pt.dir, "registry_git_pkgs") + lockPath := filepath.Join(pt.dir, "package.lock") + info, err := os.Stat(lockPath) + assert.NoError(t, err) + mtimeBefore := info.ModTime() + + pt.GoldToit("test", [][]string{ + {"// Add registry so we can find packages."}, + {"pkg", "registry", "add", "--local", "test-reg", regPath}, + {"// Just 'install' doesn't add the missing dependencies."}, + {"pkg", "install"}, + }) + + info, err = os.Stat(lockPath) + assert.NoError(t, err) + mtimeAfter := info.ModTime() + assert.Equal(t, mtimeBefore, mtimeAfter) + }) } From f1bcd2bcdf4ed9599cd8b5e03b884ca8a971281f Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 5 Jul 2024 13:13:31 +0200 Subject: [PATCH 2/3] Don't always make toit.compile absolute. That doesn't work on Windows. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d9cfe11..98463c3 100644 --- a/Makefile +++ b/Makefile @@ -2,8 +2,8 @@ BUILD_DIR := build GO_PACKAGE := github.com/toitlang/tpkg -# The TOITC_PATH needs to be absolute so that 'toit.lsp' can find it. -TOITC_PATH ?= $(shell command -v toit.compile) +# When rebuilding gold files, the TOITC_PATH must be changed to be absolute. +TOITC_PATH ?= toit.compile TOITLSP_PATH ?= toit.lsp TOITVM_PATH ?= toit.run TOITPKG_PATH ?= $(CURDIR)/$(BUILD_DIR)/toitpkg From 5d4cf6f0200489bd312d8a9b284987ec8ed1d702 Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 5 Jul 2024 14:15:38 +0200 Subject: [PATCH 3/3] Simplify. --- pkg/tpkg/lock_file.go | 52 ++++--------------------------------------- pkg/tpkg/spec.go | 42 +++++----------------------------- pkg/tpkg/util.go | 26 ++++++++++++++++++++++ 3 files changed, 36 insertions(+), 84 deletions(-) diff --git a/pkg/tpkg/lock_file.go b/pkg/tpkg/lock_file.go index a32ba89..43ab349 100644 --- a/pkg/tpkg/lock_file.go +++ b/pkg/tpkg/lock_file.go @@ -59,44 +59,6 @@ type PackageEntry struct { Prefixes PrefixMap `yaml:"prefixes,omitempty"` } -func (lf *LockFile) equals(other *LockFile) bool { - if lf.SDK != other.SDK { - return false - } - if !prefixMapEquals(lf.Prefixes, other.Prefixes) { - return false - } - if len(lf.Packages) != len(other.Packages) { - return false - } - for k, v := range lf.Packages { - otherV, ok := other.Packages[k] - if !ok || !v.equals(otherV) { - return false - } - } - return true -} - -func (pe PackageEntry) equals(other PackageEntry) bool { - if pe.URL != other.URL || pe.Name != other.Name || pe.Version != other.Version || pe.Path != other.Path || pe.Hash != other.Hash { - return false - } - return prefixMapEquals(pe.Prefixes, other.Prefixes) -} - -func prefixMapEquals(a, b PrefixMap) bool { - if len(a) != len(b) { - return false - } - for k, v := range a { - if v != b[k] { - return false - } - } - return true -} - // PrefixMap has a mapping from prefix to package-id. type PrefixMap map[string]string @@ -129,22 +91,16 @@ func ReadLockFile(path string) (*LockFile, error) { } func (lf *LockFile) WriteToFile() error { - // Check whether the file already exists and has the same content. + // Write the YAML to memory first, and then compare it with any + // existing file. // We don't want to touch files if they don't change. - if _, err := os.Stat(lf.path); err == nil { - // File exists. Check whether it has the same content. - existingLock, err := ReadLockFile(lf.path) - if err == nil && existingLock != nil && lf.equals(existingLock) { - return nil - } - } - b, err := yaml.Marshal(lf) if err != nil { return err } - return os.WriteFile(lf.path, b, 0644) + + return writeFileIfChanged(lf.path, b) } // PrintLockFile prints the contents of the lock file for the current project. diff --git a/pkg/tpkg/spec.go b/pkg/tpkg/spec.go index 1d834b7..bc5665f 100644 --- a/pkg/tpkg/spec.go +++ b/pkg/tpkg/spec.go @@ -16,6 +16,7 @@ package tpkg import ( + "bytes" "fmt" "io" "os" @@ -45,25 +46,6 @@ type SpecEnvironment struct { SDK string `yaml:"sdk,omitempty"` } -func (s *Spec) equals(other *Spec) bool { - if s.Name != other.Name || s.Description != other.Description || s.License != other.License || s.Environment.SDK != other.Environment.SDK { - return false - } - if len(s.Deps) != len(other.Deps) { - return false - } - for prefix, dep := range s.Deps { - otherDep, ok := other.Deps[prefix] - if !ok { - return false - } - if dep.URL != otherDep.URL || dep.Version != otherDep.Version || dep.Path != otherDep.Path { - return false - } - } - return true -} - // DependencyMap is a map from prefix to package. type DependencyMap map[string]SpecPackage @@ -152,27 +134,15 @@ func (s *Spec) WriteYAML(writer io.Writer) error { } func (s *Spec) WriteToFile() error { - // Check whether the file already exists and has the same content. + // Write the YAML to memory first, and then compare it with any + // existing file. // We don't want to touch files if they don't change. - if _, err := os.Stat(s.path); err == nil { - existingSpec, err := ReadSpec(s.path, nullUI{}) - if err == nil && existingSpec != nil && s.equals(existingSpec) { - return nil - } - } - - file, err := os.Create(s.path) + var b bytes.Buffer + err := s.WriteYAML(&b) if err != nil { return err } - defer func() { - e := file.Close() - if e != nil { - err = e - } - }() - - return s.WriteYAML(file) + return writeFileIfChanged(s.path, b.Bytes()) } // BuildLockFile generates a lock file using the given solution. diff --git a/pkg/tpkg/util.go b/pkg/tpkg/util.go index e0f5c75..a4da9fb 100644 --- a/pkg/tpkg/util.go +++ b/pkg/tpkg/util.go @@ -16,6 +16,7 @@ package tpkg import ( + "bytes" "fmt" "os" "path/filepath" @@ -74,3 +75,28 @@ func sdkConstraintToMinSDK(sdk string) (*version.Version, error) { } return minSDK, nil } + +func writeFileIfChanged(path string, content []byte) error { + // Check whether the file already exists and has the same content. + // We don't want to touch files if they don't change. + if _, err := os.Stat(path); err == nil { + oldContent, err := os.ReadFile(path) + if err == nil && bytes.Equal(oldContent, content) { + return nil + } + } + + file, err := os.Create(path) + if err != nil { + return err + } + defer func() { + e := file.Close() + if e != nil { + err = e + } + }() + + _, err = file.Write(content) + return err +}