feat: support for Go modules to imports [needs unit test review]

[switchupcb]

Timeline

A timeline of this pull request has been created.

• July 5, 2023: Still waiting on unit test revision.
• Jan 22, 2023: Current state and scope of PR is discussed: "UNIT TESTS THAT DON'T SETUP THE INTERPRETER CORRECTLY WILL INCORRECTLY FAIL."
• Aug 9, 2022: Debugging of CI occurs.
• Aug 6, 2022: New Implementation is developed due to oversights in the replaced and original implementation.
• June 21, 2022: Call for maintainer review after long period of inactivity. @mvertes recommends vendoring third-party imports because supporting Go modules introduces an added dependency: "It [support for third-party modules using a go package] requires discussions internally on our side."

Support Go Modules in Imports

This pull request aims to solve these issues:

• https://github.com/traefik/yaegi/issues/259
• https://github.com/traefik/yaegi/issues/300
• https://github.com/traefik/yaegi/issues/656
• https://github.com/traefik/yaegi/issues/856
• https://github.com/traefik/yaegi/issues/926
• https://github.com/traefik/yaegi/issues/300

These give the error error: unable to find source related to: "github.com/switchupcb/copygen/cli/models". Either the GOPATH environment variable, or the Interpreter.Options.GoPath needs to be set.

Problem

There were a few issues with the pkgDir method.

1. The paths printed are all backwards.
2. The function runs three times for one call? Presumably to change the separator.
3. Setting goPath from options makes the program hang.
4. References GTA5.
5. Irrelevant variable root is returned.

OUTPUT of PROBLEM CODE pkgDir(), previousRoot(), effectivePkg() RPATH generator\vendor DIR src\generator\vendor\github.com\switchupcb\copygen\cli\models goPath importPath github.com/switchupcb/copygen/cli/models effectivePkg() generator\github.com\switchupcb\copygen\cli\models RPATH vendor DIR src\vendor\github.com\switchupcb\copygen\cli\models goPath importPath github.com/switchupcb/copygen/cli/models EFFECTIVE github.com\switchupcb\copygen\cli\models RPATH vendor DIR src\vendor\github.com\switchupcb\copygen\cli\models goPath importPath github.com/switchupcb/copygen/cli/models effectivePkg() github.com\switchupcb\copygen\cli\models prevRoot interp.opt.filesystem &{} FRAG github.com\switchupcb\copygen\cli\models

THE PROGRAM HANGS ON THIS FUNCTION WHEN GOPATH IS SET

// Find the previous source root (vendor > vendor > ... > GOPATH).
func previousRoot(filesystem fs.FS, rootPath, root string) (string, error) {
    rootPath = filepath.Clean(rootPath)
    parent, final := filepath.Split(rootPath)
    parent = filepath.Clean(parent)

    // TODO(mpl): maybe it works for the special case main, but can't be bothered for now.
...

Fix

1. Remove the problem code.
2. Don't reverse the filepath.
3. Use cross-platform implementation via filepath.Join.
4. Provide more informative error messages.

Solution

Go modules are located in $GOPATH/pkg/mod. Vendor files are located somewhere in the project relative to the go.mod file. You state that "In all other cases, absolute import paths are resolved from the GOPATH and the nested "vendor" directories." Therefore, we only need to search in a few places.

1. The GOPATH src (for regular imports)
   • Installed during Go installation.
2. The GOPATH pkg/mod (for Go modules and/or "vendor" dirs)
   • Installed during go get or placed.
3. The relative case.
   • Did not touch; however the new case may implement.
4. The vendor files are in the project.

For 1, 2, and 4: https://pkg.go.dev/golang.org/x/tools/go/packages#Package gives a list of files we can filter for the absolute import path. 3 is untouched, BUT should work with the given method. For #856 specifically, the unsafe issue is fixed by searching in the GOTOOLDIR (completed with https://github.com/traefik/yaegi/pull/1265/commits/9605bc7ba2d181b6e69893d02a5f8d6c39836a99).

The following code finds a relative path's absolute path from the current working directory. The other code regarding relative path wasn't commented, so I did not implement it.

absgopath, err := filepath.Abs(rPath)
    if err != nil {
        return nil, fmt.Errorf("There was an error retrieving the absolute filepath for the relative path.". rPath)
    }

Test

If GOCACHE is not set, you will get this error:

err: exit status 1: stderr: build cache is required, but could not be located: GOCACHE is not defined and %LocalAppData% is not defined`.

While running the interpreter, you might receive

stderr: go: creating work dir: mkdir C:\WINDOWS\go-build1065008415: Access is denied.

IF an import can't be found AND you aren't in administrator (from the command line).

go test ./_test 
_test\bad0.go:1:1: expected 'package', found println
package github.com/traefik/yaegi/_test
        imports github.com/traefik/yaegi/_test/c1
        imports github.com/traefik/yaegi/_test/c2
        imports github.com/traefik/yaegi/_test/c1: import cycle not allowed
_test\pkgname0.go:4:2: no required module provides package guthib.com/bar; to add it:
        go get guthib.com/bar
_test\pkgname0.go:5:2: no required module provides package guthib.com/baz; to add it:
        go get guthib.com/baz
_test\ipp_as_key.go:5:2: no required module provides package guthib.com/tata; to add it:
        go get guthib.com/tata
_test\ipp_as_key.go:4:2: no required module provides package guthib.com/toto; to add it:
        go get guthib.com/toto

Note: Did not run install.sh cause its not supported.

Disclaimer

These changes allow me to use yaegi imports without yaegi extract.

[CLAassistant]

All committers have signed the CLA.

[switchupcb]

Update

I can confirm that the import is found. However, I still receive an error while running when I run the interpreter without the imported symbols.

panic: C:\Program Files\Go\src\unsafe\unsafe.go:192:1: CFG post-order panic: runtime error: index out of range [3] with length 3

StackTrace

Issue fixed...

[switchupcb]

This was solved with the following code:

i := interp.New(interp.Options{GoPath: os.Getenv("GOPATH"), GoCache: goCache, GoToolDir: build.ToolDir})
	i.Use(stdlib.Symbols)
	if _, err := i.Eval(source); err != nil {
		return nil, fmt.Errorf("An error occurred loading the template file: %v\n%v", absfilepath, err)
	}

Therefore, the stdlib symbols must be used to import external or vendored packages.

[switchupcb]

I'm pretty sure this pull request works. My only trouble now is with the library:

// an error occurs here.
bar := v.Interface().(func(string) string)

panic: interface conversion: interface {} is func(struct { Filepath string; Loadpath string; Template struct { Headpath string; Funcpath string }; Package string; Imports []string; Functions []struct { Name string; To []struct { Package string; Name string; VariableName string; Fields []*unsafe2.dummy; Options struct { Import string; Pointer bool; Depth int; Deepcopy string; Custom map[string]interface {} } }; From []struct { Package string; Name string; VariableName string; Fields []*unsafe2.dummy; Options struct { Import string; Pointer bool; Depth int; Deepcopy string; Custom map[string]interface {} } }; Options struct { Custom map[string]interface {} } } }) string, not func(models.Generator) string

[switchupcb]

@mvertes

[switchupcb]

Update

This pull request can use Go Module imports without an issue finding the source. If @mpl can let me know all the edge cases he is solving in importSrc and updates the tests to use importSrc instead of its sub-methods I can improve this.

Error

When you attempt to use an object (not loaded by symbols) from the import, you will receive:

panic: C:\Program Files\Go\src\unsafe\unsafe.go:192:1: CFG post-order panic: runtime error: index out of range [3] with length 3

This is solved by using the following code.

if err := i.Use(unsafe.Symbols); err != nil {
  // ...
}

[switchupcb]

When a user attempts to print an object from the Go Module, this error occurs in cfg.go at line 2629 in method arrayTypeLen(). This happens when you call fmt.Println() with a custom module type:

panic: reflect: call of reflect.Value.Int on uintptr Value

goroutine 1 [running]:
reflect.Value.Int(...)
        C:/Program Files/Go/src/reflect/value.go:1334
github.com/traefik/yaegi/interp.arrayTypeLen(0x17dad20, 0xc0001027e0)
        C:/Users/User/Documents/Github/yaegi/interp/cfg.go:2647 +0x33c
...see comment edit for full stacktrace

These users understand cfg.go: @mvertes @nrwiersma @mpl

[bobfang1992]

How far did you get with this? Is this at least working on your machine? I really hope this got more traction and get merged eventually.

[switchupcb]

How far did you get with this? Is this at least working on your machine? I really hope this got more traction and get merged eventually. @bobfang1992

I will test it with the next update of Copygen but that issue is a responsibility of those users (as they coded cfg.go which is 2730 lines) [@mvertes @nrwiersma @mpl ]. There's not really anything else I can do on my end.

[c0b41]

Any Update @mvertes

[switchupcb]

When a user attempts to print an object from the Go Module, this error occurs in cfg.go at line 475. This happens when you call fmt.Println() with a custom module type (string alias):

package template

import (
	"fmt"

	"github.com/switchupcb/copygen/cli/generator" // third party module
	"github.com/switchupcb/copygen/cli/models" //  extracted module
)

func Generate(gen *models.Generator) (string, error) {
	content := string(gen.Keep) + "\n"
	fmt.Println(generator.GenerateFunction) // custom type in third party module

	for i := range gen.Functions {
		content += Function(&gen.Functions[i]) + "\n"
	}

	return content, nil
}

Stacktrace

panic: runtime error: index out of range [3] with length 3 [recovered]
        panic: C:\Program Files\Go\src\unsafe\unsafe.go:192:1: CFG post-order panic: runtime error: index out of range [3] with length 3

goroutine 1 [running]:
github.com/traefik/yaegi/interp.(*Interpreter).cfg.func2.1()
        C:/Users/User/go/pkg/mod/github.com/traefik/yaegi/interp/cfg.go:475 +0x78
panic({0x10b9620, 0xc001408ba0})
        C:/Program Files/Go/src/runtime/panic.go:1038 +0x215
...see comment edit for full stacktrace

[zale144]

@switchupcb So I'm getting an error:

2022/03/27 13:07:50 1:21: import "gitlab.com/user/pkg" error: An import source could not be found: "gitlab.com/user/pkg"
The current GOPATH=/Users/myself/go, GOCACHE=/Users/myself/Library/Caches/go-build, GOTOOLDIR=/usr/local/go/pkg/tool/darwin_arm64
The target filepath "gitlab.com/user/pkg" is not within the path "/usr/local/go"

while setting options as such:

	i := interp.New(interp.Options{
		GoPath:    os.Getenv("GOPATH"),
		GoCache:   os.Getenv("GOCACHE"),
		GoToolDir: build.ToolDir,
		Stdout:    &stdout, Stderr: &stderr,
	})

any ideas?

[switchupcb]

@zale144 I'm not sure that os.Getenv("GOCACHE") will fetch the correct cache. Use os.UserCacheDir(). However, the actual issue says that gitlab.com/user/pkg isn't in /usr/local/go. So first check that it's there (or within a subdirectory). If it is there, there is an error. If it's not there, you need to go get it.

If it's already go get then there is some logic error that throws an error in this function. It's called from this line, which means the import couldn't be found using Go's package tool either (which handles import).

[zale144]

@zale144 I'm not sure that os.Getenv("GOCACHE") will fetch the correct cache. Use os.UserCacheDir(). However, the actual issue says that gitlab.com/user/pkg isn't in /usr/local/go. So first check that it's there (or within a subdirectory). If it is there, there is an error. If it's not there, you need to go get it.

If it's already go get then there is some logic error that throws an error in this function. It's called from this line, which means the import couldn't be found using Go's package tool either (which handles import).

Thanks for the quick reply. I tried it and the same problem persists.

[switchupcb]

@zale144 > https://github.com/traefik/yaegi/pull/1265#issuecomment-1081210351 What directory is the package you go get located on your machine?

[zale144]

@zale144 > #1265 (comment) What directory is the package you go get located on your machine?

Silly me, I haven't added the dependency in the go.mod file. However, in order to keep it there, I need to have it imported, which for me kind of defeats the purpose, so I'll just probably go with something like git submodules and have it available locally.

Though, this error remains (from the current latest):

import "github.com/spf13/cobra" error: /Users/myself/go/pkg/mod/github.com/spf13/[email protected]/args.go:61:9: cannot use  (type funcT) as type funcT in return argument

So at this point I might just get back to the go plugins torture.

Thanks anyway!

[switchupcb]

@zale144 You don't need the dependency in your go.mod file, but it has to be located on the machine. Otherwise, there is nothing to interpret.

import "github.com/spf13/cobra" error: /Users/myself/go/pkg/mod/github.com/spf13/[email protected]/args.go:61:9: cannot use (type funcT) as type funcT in return argument

This is an error indicating a problem with the package you're attempting to import. Does the import work without an interpreter? If so, the error is from the interpreter itself.

[switchupcb]

It's puwl wequest wednesday.

[tonyalaribe]

Hi, How can I help to push this forward? Should I rebase it?

These days pretty much every Go package runs on Gomodules, so it's difficult to use yaegi on any real world project without it.

[switchupcb]

@tonyalaribe Sure! I'm not sure why your comment was marked as off-topic, but I have updated the branch. Here is a description of the current state of this pull request.

TLDR: Maintainers need to approve workflows/review.

I will run another Go Module test around the release of Copygen v0.4, but the general idea is to create a file that uses a non-extracted third-party module and determine if that code runs. Note that you must go get the module prior to its non-extracted use; it doesn't need to be in the go.mod. As an example, the errors present in https://github.com/traefik/yaegi/pull/1265#issuecomment-1078521549 occur in the cfg.go file which may indicate an error with the interpretation of imported types, or just an error with the interpreter in general. Using imports with the standard library works as outlined in a disgo generate.go copygen file.

Specifically, I'm confident that the current state of this pull-request fixes the issue with importing; as the original code was platform-specific and unfinished (according to @mpl's comments). If that statement is correct, the errors I'm facing are edge cases within the interpreter, which is NOT necessarily in the scope of this pull request. In other words, this pull request is finished and those errors should be solved in a separate pull request.

The main issue is that this pull request has gone 9 months without a comment from the maintainers; let alone workflow approval. In addition, it has the "needs design review" label which seems to historically be placed on pull requests that are never merged. In this case, the best way to move this forward is to bring this pull request to the attention of the maintainers @mvertes, @mpl. A discussion involving this pull request was addressed by @ldez in https://github.com/traefik/yaegi/discussions/1292 but nothing happened since that time.

[mvertes]

First, thank you @switchupcb for your interest in yaegi and this contribution. It's really appreciated. Also please forgive my delayed response.

While it would be really nice to have direct support of Go modules in Yaegi, it comes here with a high cost (the introduction of external dependencies, something we have avoided so far), and seems not mandatory, even with the complete adoption of modules in the Go ecosystem.

The reason is that we can always perform vendoring, i.e. use the command go mod vendor in the directory of the code to pass to yaegi, thus internalise all dependencies into a vendor directory relative to the go.mod file (and this recursively for the transitive closure of dependencies). Yaegi will then load all required dependencies from the vendor subtree without having to know about modules and read / parse go.mod files. Vendoring is a major feature of the Go tooling and will not be deprecated in the foreseeable future.

Another benefit of using vendoring is to be self-contained and to know exactly the amount of code that you need to pass to the interpreter (as interpretation still has limitations and performance costs).

For example in Traefik, all plugins operate that way, and even if (or when) yaegi would support modules natively, we will still require vendoring to handle dependencies.

That being said, we understand the need in a general context. Ideally we would prefer a solution without external dependencies, but we do not know yet if it is reasonably feasible or not. If not, your solution may be a starting point, but we need to change our stance about dependencies and it requires discussions internally on our side.

We are looking at it. Thank you for your patience.

[tonyalaribe]

Thanks @switchupcb for the detailed status update and for the time you've invested into this topic.

Thanks also @mvertes for circling in here, to let us know the state of things. This puts things into better perspective. I can understand why it's benefitial to have a small/no dependency footprint, especially with the security sensitive domain traefik finds itself in. Unfortunately just speaking as a user trying to get the library work, it's pretty difficult to get it to work with the current state of things in the Go ecosystem.

For example, even when trying out the to use yaegi on a vendored project like you describe, I get errors such as

panic: doctests.go:12:2: import "github.com/kr/pretty" error: package location /Users/me/Projects/doctests not in GOPATH

When I call the EvalPath method on a vendored package with the library in question in the vendor directory.

Moviing the library into my $GOPATH doesn't still help and gives me other versions of the GOPATH errors. So even the go mod vendor workaround isn't sufficient. Maybe there's some extra necessary configuration needed. But in anycase, more instructions would need to be given to users of the library, and that approach of requiring a go mod vendor might not be sufficient.

[mvertes]

@tonyalaribe, for the problem you mention, it may be worth to open a specific issue with a minimal reproducible case, that is a main package file (hello world style), that we can both run with yaegi and go run, assuming vendoring.

The sample should at least contain the problematic import from main, and nothing much more. go.mod should be provided as well only if go mod tidy doesn't resolve from an empty go.mod.

I agree that we should better document about vendor, and ultimately remove that friction when not necessary. That will require some work.

Thanks

[switchupcb]

@ldez I rebased.

Did not fix any new linter issues that may occur as the ones I encountered were not relevant to the commits in this pull request. I am not sure how the linter passed CI for https://github.com/traefik/yaegi/runs/7013118594?check_suite_focus=true given those linter errors, but if there is a problem it is an easy fix.

[ldez]

The problem is not the linter but the merges. I will try to fix that but I recommend not merging 3 branches into one.

I also recommend not merging but rebasing your PRs and using a dedicated branch for PRs (not master or main)

[ldez]

I was forced to squash your commits.

I will insist on the fact to address changes as commits and use rebase (not squash) instead of merge.

$ git remote -v 
origin  [email protected]:switchupcb/yaegi.git (fetch)
origin  [email protected]:switchupcb/yaegi.git (push)
upstream        [email protected]:traefik/yaegi.git (fetch)
upstream        [email protected]:traefik/yaegi.git (push)

$ git fetch upstream
$ git rebase upstream/master

I created a backup of the previous content here: https://github.com/ldez/yaegi/tree/backup/switchupcb-master

[switchupcb]

Great! I can fix that.

[ldez]

To update your local branch with my changes:

$ git remote -v 
origin  [email protected]:switchupcb/yaegi.git (fetch)
origin  [email protected]:switchupcb/yaegi.git (push)
upstream        [email protected]:traefik/yaegi.git (fetch)
upstream        [email protected]:traefik/yaegi.git (push)

$ git switch master
$ git fetch origin
$ git reset --hard origin/master

[switchupcb]

To be specific, I believe build cache is required, but could not be located: GOCACHE is not defined and neither $XDG_CACHE_HOME nor $HOME are defined is related to the make environment not defining those environment variables. GOCACHE is used in this pull request. I am not sure what $XDG_CACHE_HOME nor $HOME is referencing.

In the other case, go: GOPATH entry is relative; must be absolute path: "./_pkg". is addressed in the original pull request post (https://github.com/traefik/yaegi/pull/1265#issue-1008049349).

Did not touch; however the new case may implement.

I will ensure an absolute path is used in whichever function is causing that error.

[switchupcb]

I can begin the work for those fixes tommorow.