Repository: lima-vm/lima
Pull request #2107 (2024-01-01 02:03:16)

Store Lima version in the instance directory
That way future Lima releases can make decisions about default settings based on the Lima version that created the instance instead of the current instance, and settings for existing instances will not change just because the user updated Lima.

This implements the mechanism I described in https://github.com/lima-vm/lima/pull/1953#issuecomment-1784546687. I'm leaving it in draft mode for now because I'm not sure if the `store.MinVersion` function is the right way to use the data; I originally wanted to make it an instance method, but then noticed that most location that would need to call it don't have a copy of the instance.

So this PR probably needs to wait until we have a companion PR that makes use of this new mechanism. Or I could drop the `MinVersion` function for now and leave it for the first PR that will take advantage of the data.

```console
$ limactl ls --format '{{.Name}} - {{.LimaVersion}}'
alpine - v0.19.1
default - v0.19.1-16-gf3dc6ed.m
```

Either way, feedback welcome!

jandubois: @AkihiroSuda Thank you for your feedback; I believe I've addressed all your comments.

I switched to defining a `LimaVersionGreaterThan` function, so can select default changes based on the last version that still has the old functionality instead of having to guess the next release version number that will have the new default. Still needs to be validated by an actual implementation that uses it though.

```
if LimaVersionGreaterThan(inst.LimaVersion, "0.19.1") {
   // set new default
}
```

This code would use the new defaults with any current dev build because `v0.19.1-xxx` will be treated as greater than `0.19.1`.

This should also make it easier to test the new functionality in CI without having to bump the version first.

It does mean though that all instances created by any dev build of Lima after `0.19.1` will be treated as if it was created by the latest build. I think this makes sense, as Lima developers should understand what is going to happen. Let me know what you think!
jandubois: Current failure happens because the CI version has no tags, so the version cannot be parsed from the SHA1:

```
time="2024-01-02T05:59:07Z" level=fatal msg="errors inspecting instance: [cannot parse lima version \"ca7752d\" from \"/Users/runner/.lima/default/lima-version\": ca7752d is not in dotted-tri format]"
```

Not sure if we should pull tags in CI, so we can then test version-specific changes?
AkihiroSuda: > Not sure if we should pull tags in CI, so we can then test version-specific changes?

We may pull the tags, but anyway the commit hash string shouldn't cause a fatal error.

jandubois: > We may pull the tags, but anyway the commit hash string shouldn't cause a fatal error.

The problem is that we won't know the version, so all the version check will return `false` then. Instead of the newest defaults, we get the oldest ones. Therefore I kind of prefer that this fails in CI instead of silently testing the old values.

We can make this configurable, but it feels like overkill: why do we need to support building outside of a git checkout?
AkihiroSuda: > The problem is that we won't know the version, so all the version check will return false then. Instead of the newest defaults, we get the oldest ones. Therefore I kind of prefer that this fails in CI instead of silently testing the old values.

I think the version without semver prefix can be just assumed to be "latest", with a warning message.
For the CI, we can pull the tags to attach the semver prefix.

> why do we need to support building outside of a git checkout?

Because somebody might be using a package built in that way?



Patch:
diff --git a/cmd/limactl/start.go b/cmd/limactl/start.go
index 19d26f98..13a8ae1a 100644
--- a/cmd/limactl/start.go
+++ b/cmd/limactl/start.go
@@ -23,6 +23,7 @@ import (
 	"github.com/lima-vm/lima/pkg/store/filenames"
 	"github.com/lima-vm/lima/pkg/templatestore"
 	"github.com/lima-vm/lima/pkg/uiutil"
+	"github.com/lima-vm/lima/pkg/version"
 	"github.com/lima-vm/lima/pkg/yqutil"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -332,6 +333,9 @@ func createInstance(ctx context.Context, st *creatorState, saveBrokenEditorBuffe
 	if err := os.WriteFile(filePath, st.yBytes, 0o644); err != nil {
 		return nil, err
 	}
+	if err := os.WriteFile(filepath.Join(instDir, filenames.LimaVersion), []byte(version.Version), 0o444); err != nil {
+		return nil, err
+	}
 
 	inst, err := store.Inspect(st.instName)
 	if err != nil {
diff --git a/pkg/store/filenames/filenames.go b/pkg/store/filenames/filenames.go
index 466d552d..6615b410 100644
--- a/pkg/store/filenames/filenames.go
+++ b/pkg/store/filenames/filenames.go
@@ -27,6 +27,7 @@ const (
 
 const (
 	LimaYAML           = "lima.yaml"
+	LimaVersion        = "lima-version" // Lima version used to create instance
 	CIDataISO          = "cidata.iso"
 	CIDataISODir       = "cidata"
 	BaseDisk           = "basedisk"
diff --git a/pkg/store/instance.go b/pkg/store/instance.go
index 27e09d6e..e0161032 100644
--- a/pkg/store/instance.go
+++ b/pkg/store/instance.go
@@ -16,12 +16,14 @@ import (
 	"text/template"
 	"time"
 
+	"github.com/coreos/go-semver/semver"
 	"github.com/docker/go-units"
 	hostagentclient "github.com/lima-vm/lima/pkg/hostagent/api/client"
 	"github.com/lima-vm/lima/pkg/limayaml"
 	"github.com/lima-vm/lima/pkg/store/dirnames"
 	"github.com/lima-vm/lima/pkg/store/filenames"
 	"github.com/lima-vm/lima/pkg/textutil"
+	"github.com/sirupsen/logrus"
 )
 
 type Status = string
@@ -56,6 +58,7 @@ type Instance struct {
 	Config          *limayaml.LimaYAML `json:"config,omitempty"`
 	SSHAddress      string             `json:"sshAddress,omitempty"`
 	Protected       bool               `json:"protected"`
+	LimaVersion     string             `json:"limaVersion"`
 }
 
 func (inst *Instance) LoadYAML() (*limayaml.LimaYAML, error) {
@@ -167,6 +170,16 @@ func Inspect(instName string) (*Instance, error) {
 			}
 		}
 	}
+
+	limaVersionFile := filepath.Join(instDir, filenames.LimaVersion)
+	if version, err := os.ReadFile(limaVersionFile); err == nil {
+		inst.LimaVersion = strings.TrimSpace(string(version))
+		if _, err = parseLimaVersion(inst.LimaVersion); err != nil {
+			logrus.Warnf("treating lima version %q from %q as very latest release", inst.LimaVersion, limaVersionFile)
+		}
+	} else if !errors.Is(err, os.ErrNotExist) {
+		inst.Errors = append(inst.Errors, err)
+	}
 	return inst, nil
 }
 
@@ -423,3 +436,36 @@ func (inst *Instance) Unprotect() error {
 	inst.Protected = false
 	return nil
 }
+
+// parseLimaVersion parses a Lima version string by removing the leading "v" character and
+// stripping everything from the first "-" forward (which are `git describe` artifacts and
+// not semver pre-release markers). So "v0.19.1-16-gf3dc6ed.m" will be parsed as "0.19.1".
+func parseLimaVersion(version string) (*semver.Version, error) {
+	version = strings.TrimPrefix(version, "v")
+	version, _, _ = strings.Cut(version, "-")
+	return semver.NewVersion(version)
+}
+
+// LimaVersionGreaterThan returns true if the Lima version used to create an instance is greater
+// than a specific older version. Always returns false if the Lima version is the empty string.
+// Unparsable lima versions (like SHA1 commit ids) are treated as the latest version and return true.
+// limaVersion is a `github describe` string, not a semantic version. So "0.19.1-16-gf3dc6ed.m"
+// will be considered greater than "0.19.1".
+func LimaVersionGreaterThan(limaVersion, oldVersion string) bool {
+	if limaVersion == "" {
+		return false
+	}
+	version, err := parseLimaVersion(limaVersion)
+	if err != nil {
+		return true
+	}
+	switch version.Compare(*semver.New(oldVersion)) {
+	case -1:
+		return false
+	case +1:
+		return true
+	case 0:
+		return strings.Contains(limaVersion, "-")
+	}
+	panic("unreachable")
+}
diff --git a/website/content/en/docs/dev/Internals/_index.md b/website/content/en/docs/dev/Internals/_index.md
index 1089a83c..c1cd06ac 100644
--- a/website/content/en/docs/dev/Internals/_index.md
+++ b/website/content/en/docs/dev/Internals/_index.md
@@ -30,6 +30,7 @@ having to specify an identity explicitly.
 An instance directory contains the following files:
 
 Metadata:
+- `lima-version`: the Lima version used to create this instance
 - `lima.yaml`: the YAML
 - `protected`: empty file, used by `limactl protect`
 


Test patch:
diff --git a/pkg/store/instance_test.go b/pkg/store/instance_test.go
index bdbbba7d..0d532673 100644
--- a/pkg/store/instance_test.go
+++ b/pkg/store/instance_test.go
@@ -155,3 +155,12 @@ func TestPrintInstanceTableTwo(t *testing.T) {
 	assert.NilError(t, err)
 	assert.Equal(t, tableTwo, buf.String())
 }
+
+func TestLimaVersionGreaterThan(t *testing.T) {
+	assert.Equal(t, LimaVersionGreaterThan("", "0.1.0"), false)
+	assert.Equal(t, LimaVersionGreaterThan("0.0.1", "0.1.0"), false)
+	assert.Equal(t, LimaVersionGreaterThan("0.1.0", "0.1.0"), false)
+	assert.Equal(t, LimaVersionGreaterThan("0.1.0-2", "0.1.0"), true)
+	assert.Equal(t, LimaVersionGreaterThan("0.2.0", "0.1.0"), true)
+	assert.Equal(t, LimaVersionGreaterThan("abacab", "0.1.0"), true)
+}