pkg/usrlocalsharelima: simplify and allow multiple paths, etc.

[AkihiroSuda]

Contains

• Fix libexec dir detection on Homebrew #4296

[jandubois]

I don't think "simplify" correctly describes all the changes related to delve; maybe they should be in a separate PR?

I wonder if this could be generalized to also cover calling usrlocal.Prefix() from unit test. This currently also fails because os.Executable() is somewhere under $TMPDIR.

I think the shareDirs should be added to the limactl info output.

[AkihiroSuda]

I don't think "simplify" correctly describes all the changes related to delve; maybe they should be in a separate PR?

Done

I wonder if this could be generalized to also cover calling usrlocal.Prefix() from unit test. This currently also fails because os.Executable() is somewhere under $TMPDIR.

Which test?

I think the shareDirs should be added to the limactl info output.

Debug log may suffice? Can be a post-2.0 PR?

[jandubois]

I don't think "simplify" correctly describes all the changes related to delve; maybe they should be in a separate PR?

Done

The delveWorkspace etc changes are still in the "simplify" commit.

I wonder if this could be generalized to also cover calling usrlocal.Prefix() from unit test. This currently also fails because os.Executable() is somewhere under $TMPDIR.

Which test?

There is no test using it because it doesn't work. I ran into this 2 or 3 times, and just rewrote the code so that the test would not hit the code path that calls usrlocallimashare.Dir().

So this is not urgent. But if you fix it for delve, why not fix it for go test as well?

I think the shareDirs should be added to the limactl info output.

Debug log may suffice? Can be a post-2.0 PR?

Yes, can be done later. Seems trivial though, so I might just create a PR.

[AkihiroSuda]

The delveWorkspace etc changes are still in the "simplify" commit.

Yes, as intended. It is not new to respect the delve workspace.

But if you fix it for delve, why not fix it for go test as well?

Which test?

[jandubois]

The delveWorkspace etc changes are still in the "simplify" commit.

Yes, as intended. It is not new to respect the delve workspace.

That does not seem correct. There is no reference to delve in the pkg/ tree at all before this commit.

And most of the code of the "simplify and allow multiple paths" commit is about refactoring, adding delveDebugExe, delveWorkspace etc, which are actually making the code more complex and not simpler.

Maybe we should merge the PR as-is in the name of expediency, but the content of the commit does not match the commit summary, which only talks about the renaming of the package.

But if you fix it for delve, why not fix it for go test as well?

Which test?

As I mentioned, there is no test because it doesn't work. Just for demonstration I've added a call to ptr_test.go because it is otherwise a trivial file:

--- pkg/ptr/ptr_test.go
+++ pkg/ptr/ptr_test.go
@@ -6,10 +6,13 @@ package ptr
 import (
 	"testing"

+	"github.com/lima-vm/lima/v2/pkg/usrlocalsharelima"
 	"gotest.tools/v3/assert"
 )

 func TestOf(t *testing.T) {
+	_, err := usrlocalsharelima.Dir()
+	assert.NilError(t, err)
 	assert.DeepEqual(t, bool(true), *Of(true))
 	assert.DeepEqual(t, int(10), *Of(10))
 	assert.DeepEqual(t, string(""), *Of(""))

This always fails:

❯ go test ./pkg/ptr
--- FAIL: TestOf (0.00s)
    ptr_test.go:15: assertion failed: error is not nil: failed to find "lima-guestagent.Linux-aarch64" binary for [/var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/b001 /private/var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/b001], attempted [/var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/b001/lima-guestagent.Linux-aarch64 /var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/share/lima/lima-guestagent.Linux-aarch64 /private/var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/b001/lima-guestagent.Linux-aarch64 /private/var/folders/ds/jk3wz4n96d54wp16s0tg7d2r0000gn/T/go-build1225208507/share/lima/lima-guestagent.Linux-aarch64]
FAIL
FAIL	github.com/lima-vm/lima/v2/pkg/ptr	0.199s
FAIL

Right now it is impossible to write a unit test for any code that calls the usrlocalsharelima functions.

[AkihiroSuda]

There is no reference to delve in the pkg/ tree at all before this commit.

There is, although abbreviated as dlv

lima/pkg/usrlocalsharelima/usrlocalsharelima.go

Lines 105 to 113 in 6c1d2f7

	if debugutil.Debug {
	// candidate 2: launched by `~/go/bin/dlv dap`
	// - self: ${workspaceFolder}/cmd/limactl/__debug_bin_XXXXXX
	// - agent: ${workspaceFolder}/_output/share/lima/lima-guestagent.Linux-x86_64
	// - dir: ${workspaceFolder}/_output/share/lima
	candidateForDebugBuild := filepath.Join(filepath.Dir(selfDirDir), "_output/share/lima/lima-guestagent."+ostype+"-"+arch)
	gaCandidates = append(gaCandidates, candidateForDebugBuild)
	logrus.Infof("debug mode detected, adding more guest agent candidates: %v", candidateForDebugBuild)
	}

[jandubois]

I'm going to approve in case you want to merge right now for the 2.0 release.

I feel the whole delve logic should be documented instead of having URLs pointing to commits or issue comments. It is not self-evident why this code is needed.