Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

switchupcb
Copy link

@switchupcb switchupcb commented Sep 27, 2021

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:

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 9605bc7).

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
Copy link

CLAassistant commented Sep 27, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ldez
❌ switchupcb


switchupcb seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@switchupcb switchupcb changed the title feat: add partial support for Go modules to imports feat: add support for Go modules to imports Sep 27, 2021
@switchupcb
Copy link
Author

switchupcb commented Sep 28, 2021

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

@mvertes

@switchupcb switchupcb changed the title feat: add support for Go modules to imports add support for Go modules to imports Oct 10, 2021
@switchupcb switchupcb changed the title add support for Go modules to imports fix support for Go modules to imports Oct 10, 2021
@switchupcb
Copy link
Author

switchupcb commented Oct 19, 2021

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
Copy link
Author

switchupcb commented Oct 26, 2021

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
Copy link

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
Copy link
Author

switchupcb commented Jan 18, 2022

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
Copy link

c0b41 commented Feb 12, 2022

Any Update @mvertes

@switchupcb switchupcb marked this pull request as draft March 24, 2022 22:56
@switchupcb
Copy link
Author

switchupcb commented Mar 25, 2022

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

@switchupcb switchupcb marked this pull request as ready for review March 25, 2022 00:12
@zale144
Copy link

zale144 commented Mar 27, 2022

@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
Copy link
Author

switchupcb commented Mar 28, 2022

@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
Copy link

zale144 commented Mar 28, 2022

@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
Copy link
Author

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

@zale144
Copy link

zale144 commented Mar 29, 2022

@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/cobra@v1.4.0/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
Copy link
Author

@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/cobra@v1.4.0/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
Copy link
Author

It's puwl wequest wednesday.

@tonyalaribe
Copy link

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
Copy link
Author

switchupcb commented Jun 22, 2022

@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 #1265 (comment) 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 #1292 but nothing happened since that time.

@ldez
Copy link
Member

ldez commented Jul 29, 2022

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  git@github.com:switchupcb/yaegi.git (fetch)
origin  git@github.com:switchupcb/yaegi.git (push)
upstream        git@github.com:traefik/yaegi.git (fetch)
upstream        git@github.com: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

@ldez ldez force-pushed the master branch 2 times, most recently from b154b4e to 4b1426b Compare July 29, 2022 19:36
@switchupcb
Copy link
Author

Great! I can fix that.

@ldez
Copy link
Member

ldez commented Jul 29, 2022

To update your local branch with my changes:

$ git remote -v 
origin  git@github.com:switchupcb/yaegi.git (fetch)
origin  git@github.com:switchupcb/yaegi.git (push)
upstream        git@github.com:traefik/yaegi.git (fetch)
upstream        git@github.com:traefik/yaegi.git (push)

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

@switchupcb
Copy link
Author

switchupcb commented Jul 29, 2022

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 (#1265 (comment)).

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
Copy link
Author

I can begin the work for those fixes tommorow.

@switchupcb
Copy link
Author

switchupcb commented Aug 6, 2022

Started the work now.

Realization

What I have realized is that both the old and new approaches simply address the happy path, but don't fundamentally address the main issue. The old approach by @mpl attempts to do what the packages.Load() function does, with some extra details regarding relative paths. The new method uses packages.Load(), then attempts to fill a gap — caused by a lack of environment variables — with remnants of the old method.

I looked at the packages module to view what is required to load packages.

Basically, these 6 environment variables can determine how a module is loaded, and where it needs to be loaded from.

Issue Revisited

In the current version of yaegi, the user is advised to set the GOPATH to allow usage of modules. This may work in some cases, where the GOPATH is able to resolve to the exact package it's importing. The issue is that this is likely not the case. As an example, using go get github.com\switchupcb\copygen@v0.4.0 will create a directory in go\pkg\mod\github.com\switchupcb\copygen@v0.4.0, while the importPath may be github.com/switchupcb/copygen/cli/models. In this case, there is no github.com/switchupcb/copygen/cli/models, only \github.com\switchupcb\copygen@v0.4.0\cli\models, so the import fails; regardless of the method being used.

Adding the GOCACHE can help the packages module find the actual package, which is why it is useful to add. This situation is the case for most of the "Required Environment Variables" which is why they aren't always required. In this PR, GOTOOLDIR is set, but not actually used by packages module. Instead, it is used by an approach of finding packages inspired by the "remnants of the old method". In hindsight, I don't recommend requiring GOTOOLDIR if it isn't required.

The following code represents the required variables that are passed into packages.Load(&config, path) from the config.Env []string{}.

vars := map[string]*string{
  "GOCACHE":     &envVars.gocache,
  "GOPATH":      &envVars.gopath,
  "GOROOT":      &envVars.goroot,
  "GOPRIVATE":   &envVars.goprivate,
  "GOMODCACHE":  &envVars.gomodcache,
  "GO111MODULE": &envVars.go111module,
}

The final issue is that the tests (of yaegi) do not cover every detail involved with importing a module. It is no surprise that there were issues with the old approach, and no surprise that the current approach is flawed.

Solution

As a result, a real solution that will work in every case is allowing the user to define these 6 environment variables as required.

Implementation

  1. User can pass environment variables from an Options struct which is passed to the Interpreter in New() at Line 367.
  2. A packages.Config.Env field is instantiated using interp.opt.env["environment variable"]. Note that — in theory — interp.context.GOPATH is the same GOPATH as interp.opt.env["GOPATH"]. In actuality, a user may wish to specify a different GOPATH to import modules from.
  3. Package is loaded using the absolute import path.

TODO

  • update README Documentation for Go Module Support Section (specifying how to set environment variables from New()).
  • set Required Environment Variables for import tests (GOCACHE) in Makefile such that tests pass.

@switchupcb switchupcb marked this pull request as draft August 6, 2022 23:51
@switchupcb
Copy link
Author

For backwards compatibility purposes, I have left the Interpreter Options struct with its GoPath field that is used throughout the program. However, this field could be removed and replaced with Options.Env and set accordingly in New() to address 2. in #1265 (comment).

NOTE: This is related to Line 170 of src.go

@switchupcb
Copy link
Author

I am not confident in setting the MAKEFILE according to the repository standard. I ask that @ldez or another maintainer complete that task (context in #1265 (comment) and #1265 (comment)). In any case, I can run another integration test to create a valid README example for the documentation around the release of Copygen 0.5. As a result, I have re-marked this PR as a draft (since it won't pass CI without the correct environment variables).

@ldez
Copy link
Member

ldez commented Aug 7, 2022

@switchupcb I will once again clean your PR, please read my previous comment to learn how to rebase.

Note: the GOPATH mode must be kept because it's a requirement for the plugin system of Traefik.

@ldez
Copy link
Member

ldez commented Aug 7, 2022

I was once again forced to squash your commits.
Please learn how to do a clean rebase.

To update your local branch with my changes, you can follow this previous comment.

@switchupcb
Copy link
Author

switchupcb commented Aug 10, 2022

@ldez Great, thanks. Steps didn't work the first time for some reason.

The reason that the tests for CI didn't pass is described.

1.

err: exit status 2: stderr: go: GOPATH entry is relative; must be absolute path: "./_pkg".

As an example, closure_test.go TestFunctionCall contains the following code.

i := interp.New(interp.Options{GoPath: "./_pkg"})
if err := i.Use(stdlib.Symbols); err != nil {
   t.Fatal(err)
}

Solution

Obviously, the complaint from the packages.Load call is that the environment variable is relative. This is fixed by converting a relative GOPATH to absolute when applicable.

2

err: exit status 1: stderr: build cache is required, but could not be located: GOCACHE is not defined and neither $XDG_CACHE_HOME nor $HOME are defined

As stated in #1265 (comment), this error occurs because the Github Runner (not Makefile) has an undefined $GOCACHE (or $HOME) environment variable.

Solution

Setup GOCACHE in Github Action. Use it in the test files.

- name: Setup GOCACHE
   run: go env -w GOCACHE=${{ github.workspace }}/<INSERT CACHE DIR HERE>

I don't have an Ubuntu or Linux so I am not sure about the value to expect for the "CACHE DIR". I guessed using a setup go guide; so if it's wrong it will throw a (different) error.

Other

Following these fixes, it becomes obvious why the administrator error occurs. When GOCACHE is set, packages.Load will attempt to create a file that requires privileges.

err: exit status 1: stderr: go: creating work dir: mkdir C:\WINDOWS\go-build3375461458: Access is denied.

Solution

Run in administrator (or in a mode that gives file creation permissions in that directory). Once I did this, the only issue I experienced was not having the actual package I needed to be imported.

Alternatively, I'd experience issues with tests such as TestPackagesError/different_packages_in_the_same_directory which gives the following error.

pkg_test.go:188: got "1:21: import \"github.com/foo/pkg\" error: an import source could not be found for \"github.com/foo/pkg\"", want "1:21: import \"github.com/foo/pkg\" error: found packages pkg and pkgfalse in _pkg9\\src\\github.com\\foo\\pkg"

It wants a specific string for the error but got a different error. In this case, it's obvious that the issue comes from running the test without the fake package. However, I've noted this issue in case a mismatch in error formatting is present in the next CI run.

Disclaimer

Even with the GOCACHE environment variable set, it must be passed to interp.opt.env["GOCACHE"] using the New(Options{...}. Unfortunately, the tests do NOT use the interp.New function which sets the interp.opt.env accordingly. I will not be able to fix that issue (originating from unit test code) in a timely manner. Moreso, I don't want to edit unit tests without full context to what is being tested.

In other words, UNIT TESTS THAT DON'T SETUP THE INTERPRETER CORRECTLY WILL INCORRECTLY FAIL with err: exit status 1: stderr: build cache is required, but could not be located: GOCACHE is not defined and neither $XDG_CACHE_HOME nor $HOME are defined. CI is expected to throw this error in the current state of the PR.

In addition, I do not have the fake packages and don't want to go through the effort of setting them up. cmake (for Windows) has an issue with the go generate call and I'm too lazy to set up WSL2 for this. I'm going to rely on the CI to make edits, but it requires workflow approval every time.

As a result of the above issues, it may be a better idea may be to have an actual maintainer complete the PR from here.

@switchupcb
Copy link
Author

switchupcb commented Jan 22, 2023

does your (switchupcb master branch) fork of yaegi support external modules?

To answer the quoted question in full, please consider the following information.

The main master branch of yaegi (unforked) does not allow you to import third party modules: Any attempt to do so will be met with an error. This PR (fork) will allow you to import those third party modules, but does not guarantee anything beyond that (i.e usage of the imported code).

For the switchupcb master branch of yaegi (fork), #1265 (comment) goes into depth on the limitations of third party module imports. In short, you must have valid permissions on the machine (of the interpreter), and must pass valid Go environment variables to the interpreter via the interp.Options struct during the interp.New instantiation. If these conditions are met, you will be able to import third party modules via the interpreter.

Will the import work? Yes.

Will the code you imported work? I'm not sure. I haven't tested it in a thorough manner.

Contribution

If you are interested in contributing to this pull request, what needs to be done?

You must fix the unit tests such that they use interp.New to set up the interpreter. See the Disclaimer of #1265 (comment) for more information about this pull request in relation to yaegi's unit tests. You can also read #1265 (comment) for information on the current implementation of this feature.

This can be done with the following steps.

  1. Rebase the branch.
  2. Fix new conflicts.
  3. Add changes (unit test replace).
  4. Pass CICD workflows.

After these steps, the scope of this pull request is technically completed.

@james-lawrence
Copy link

james-lawrence commented Jan 28, 2023

dunno if this helps anyone but I've been carrying a patch for yaegi to support go modules go 2ish years which uses golang builtin tooling to resolve imports. (along with another fix for an erroneous error message that claims imports already imported when an import fails to import properly)

I think the long term solution here will be to allow users to provide a resolving function. and have a the current implementation as the default and the go modules one as a swap in replacement.

I'd be willing to do the lifting to get it over the line if there is interest.

@switchupcb
Copy link
Author

I'd be willing to do the lifting to get it over the line if there is interest
@james-lawrence

Great, all you have to do is fix up the unit tests as stated in #1265 (comment). The root method (of the old implementation) isn't sufficient (and doesn't work on Windows) for reasons explained in #1265 (comment).

I think the long term solution here will be to allow users to provide a resolving function. and have a the current implementation as the default and the go modules one as a swap in replacement.
@james-lawrence

The current implementation allows users to provide the environment variables required to resolve imports correctly. This is how yaegi currently has users import modules (by setting the interp.Options). This behavior is also supported in the current implementation of this PR.

@mewmew
Copy link

mewmew commented Mar 23, 2023

I'd be willing to do the lifting to get it over the line if there is interest.

Great work @james-lawrence! There's definitely interest to have Go mod support in Yaegi! What is needed to get your local branch up to speed with the latest version of Yaegi?

Cheers,
Robin

@james-lawrence
Copy link

@mewmew its up to date. i just havent had the time to sit down and make a PR and ensure I still had the test hanging around. likely won't get to this for awhile given current life plans.

@mewmew
Copy link

mewmew commented Mar 24, 2023

@mewmew its up to date. i just havent had the time to sit down and make a PR and ensure I still had the test hanging around. likely won't get to this for awhile given current life plans.

That's fair. Life comes first.

Wish you a good start of the Spring James!

Cheers,
Robin

@switchupcb
Copy link
Author

As a reminder, this pull request is waiting on "UNIT TESTS THAT DON'T SETUP THE INTERPRETER CORRECTLY" (by not using interp.New) to be fixed. Read #1265 (comment) for details.

I will review this pull request again prior to the next update of Copygen which is coming soon.

@switchupcb switchupcb changed the title feat: support for Go modules to imports feat: support for Go modules to imports [needs unit test review] Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants