cosmos72 / gomacro

Interactive Go interpreter and debugger with REPL, Eval, generics and Lisp-like macros
Mozilla Public License 2.0
2.18k stars 94 forks source link

Gomacro and local modules #82

Closed au-phiware closed 2 years ago

au-phiware commented 4 years ago

I've been attempting to generate code in a module that reflects on itself. I don't know if I'm just going about it wrong but I suspect there is a bug so I created a repro repo: https://github.com/au-phiware/gomacro-issue-82. When running gomacro -w it appears to fall over on the import statement for the module in the working directory (with error loading package). I noticed that golang.org/x/tools/go/packages is being used and thought that there should be no reason for not finding it. So I took a closer look and tried removing the explicit Dir Config field; this allowed the package to be found, only to be not found again at a later stage, when the genimport plugin is built. This lead me to jump into the residual directory created by genimport (i.e. $GOPATH/src/gomacro.imports/...) and start hacking at the go.mod file. I stripped it back to just the module directive and added a replace directive:

module gomacro.imports/github.com/au-phiware/gomacro-issue-82

replace github.com/au-phiware/gomacro-issue-82 => /home/corin/src/github.com/au-phiware/gomacro-issue-82

This now built successfully with GO111MODULE=on go build -buildmode=plugin.

In conclusion it seems that it would be prudent for genimport to add that replace directive.

Another thing I've noticed is that in more complex modules that use replace directives in their own go.mod then I see strange errors about missing types and such (although this may be caused by the working directory having being set to /).

cosmos72 commented 4 years ago

You are right that gomacro uses golang.org/x/tools/go/packages to import packages. That's because it is the only (as far as I know) API that supports Go 1.11 modules.

I configured it with explicit Dir Config field set to "/" otherwise it will load and modify any go.mod file found in the current directory. Alas, such configuration has the side effect that only modules downloadable from internet can be imported.

As a workaround, you can toggle modules support on and off (when turned off, gomacro uses the old mechanism that searches in $GOPATH) with the command :option Import.Uses.Module

If you have suggestions on how to transparently support "private" modules (i.e. not downloadable from internet) you are welcome :)

About https://github.com/au-phiware/gomacro-issue-82: I did not understand what the code is expected to do. If you explain, I will try to help.

au-phiware commented 4 years ago

Ah, I can see how modifying the local go.mod/go.sum files would be undesirable... although in my use case I think it would be okay (not sure).

When I try using :option Import.Uses.Module, gomacro can't find the descriptor package:

error loading package "github.com/golang/protobuf/descriptor" metadata, maybe you need to download (go get), compile (go build) and install (go install) it? can't find import: "github.com/golang/protobuf/descriptor"
gen.gomacro:15:13: undefined "descriptor" in descriptor.ForMessage <*ast.SelectorExpr>

I tried the go get/install dance... but to no avail.

I've pushed another commit to au-phiware/gomacro-issue-82 with a more detailed README and uses the latest gomacro version.

I think that gomacro should treat package statements that begin with . as a special case. For instance, my package has a Person type, so I should be able to do something like this without importing the current package:

:package "."
:import "go/ast"
:macro make() ast.Node {
    x := &Person{}
    // ...
    return
}

Currently, gomacro complains about undefined identifier: Person...

au-phiware commented 4 years ago

I've come across another issue where genimport generates an incomplete go.mod file. It occurs when a dependency is shared between the process that is hosting the gomacro interpreter and the gomacro file that the interpreter is processing. As a test case you could have a gomacro file that imports github.com/mattn/go-runewidth, which would trigger genimport to call go to build the plugin, go would download the latest version (0.0.8), and happily build the plugin. But when gomacro goes to call plugin.Open it would raise an error similar to the below because gomacro was built with v0.0.7!

pkg/test.gomacro:11:2: error loading plugin "~/go/src/gomacro.imports/github.com/au-phiware/gomacro-issue-82/gomacro-issue-82.so": plugin.Open("/go/src/gomacro.imports/github.com/au-phiware/gomacro-issue-82"): plugin was built with a different version of package github.com/mattn/go-runewidth

We need some way of specifying a portion of the go.mod file that genimport generates. Maybe a package var in genimport? In that way the host program that is running the interpreter can pass in all the require and replace directives of its own go.mod file.

Let me know if you think a package var is appropriate and I'll put together a PR.

benjaminjkraft commented 4 years ago

I'm wondering about the current status of this issue. It seems like some of the import machinery has changed since the previous discussion, but importing local modules is still not supported. (Note that private modules do work fine as long as git has the right credentials, but modules (or versions) that don't exist on github are the real problem. For our use case, even if the commit existed, the repository is very large so refetching it is super slow!)

Anyway, I'm wondering if that's now more possible to fix. I found two ways to do so, and got both working in a hacky state.

One way, which is easy to implement, but a bit messy, is to just put gomacro.imports directory within the current module. It's perhaps bad hygiene to leave that behind, but if we don't worry about that, everything seems to just work. I put up a proof of concept.

Another way, which is a bit more work, although not unreasonable, uses replace to point Go at the original version of the current module. This is a bit annoying because the module-within-module setup means we have to copy over any replace directives (they are only used when in the main module). But it seems a lot cleaner overall. See the proof of concept.

If it's helpful, I can try to clean one of these up (pointers to what else I might need to change or test are very welcome as I don't know it at all). As I said this is very useful for using code that's not available from github, or to avoid refetching it.

benjaminjkraft commented 3 years ago

@cosmos72 any thoughts on the above? I'm happy to clean one of the diffs up if you'd like.

cosmos72 commented 3 years ago

Thanks for the effort and the proposals! I will read them as soon as possible, and come back with feedback

cosmos72 commented 3 years ago

@au-phiware I think there is a bug in https://github.com/au-phiware/gomacro-issue-82

The line desc, _ := descriptor.ForMessage(tutorial.Person) tries to use the type tutorial.Person as argument to the function descriptor.ForMessage. But Go types are not first class, i.e. they are not values: they cannot be passed to functions, stored in variables, returned, etc.

In fact, descriptor.ForMessage expects a descriptor.Message as argument. I am not familiar enough with protobuf to suggest a fix...

cosmos72 commented 3 years ago

about @benjaminjkraft proof of concept

with the gradual phase out of GOPATH, local modules could be stored anywhere on disk, so we need a mechanism to distinguish the command "please download, compile and import the remote package some.remote/pkg" from "please compile and import local package my.local/pkg that is conveniently located in the directory my/local/path"

Now, the Go import directive allows specifying only one path per package.

Thus either we create a different syntax to import local packages (a very general solution, but probably ugly and not intuitive to use), or we specify only the directory where a local package is located, and retrieve the package path by reading the file go.mod in such directory.

The second alternative surely seems cleaner and more intuitive. It only has a little missing point: how do we distinguish importing a remote package i.e. import "some.remote/pkg" from importing a local package from a directory i.e. import "my/local/path" ?

@au-phiware suggested to treat import "." as a special case. That's surely feasible, but in my opinion it's also somewhat limited.

What about treating as a special case also all imports starting with "./" or "../" ?

For example, one could write import "./my/local/path" and gomacro would interpret it as "please compile and import the local package located in the directory ./my/local/path and retrieve the package path from the file go.mod located there"

In my opinion it has some nice properties:

  1. it can import local modules from any directory
  2. the prefixes ./ and ../ suggest "from the current directory" and "from the parent directory", which is exactly what the import will do. Thus it also conveys the message that whatever follows is not a remote host name.
  3. If I remember correctly, Go forbids import paths starting with "." so there is no ambiguity

If your idea was to create gomacro reflection files directly inside the local module, then compile and load the local module as a plugin, please note that only packages named 'main' can be compiled as plugins - quite limiting.

I think it's better to create reflection files under the usual $GOPATH/src/gomacro.imports/... and refer to the local module using replace directives in the generated go.mod.

benjaminjkraft commented 3 years ago

Oh interesting points -- thanks for all the thoughts!

I guess the way I was thinking about it is from the point of view of the case where the current directory is inside a module, and the package we are importing is in that module. In that case, we don't need to do anything smart, just do what the rest of the go tooling does -- find the (nearest-ancestor) go.mod, and look at that. To me it's also the least surprising thing, and it's sorta parallel to what go run does [1], but perhaps my expectations are unusual, and/or perhaps this is not a complete solution or too magical.

I'd be fine with supporting import "./local/file/path" in addition or instead. (As long as that causes that package's imports to be looked up within the current module (e.g. if that file imports my.site/path/to/current/module it should use the current module for that; and probably also for dep versions) but I assume that's plenty possible and in fact maybe the natural behavior.) Certainly that is more flexible as it allows you to import any module you have locally, not just the ones in prod. I'd lean towards supporting both, but I'm ok either way. (In some ways the dot prefix is more convenient anyway.)

You definitely point out some good reasons why the second implementation is better. It also seems like it will be a bit more flexible, in that these choices seem to map more clearly to the code. It should be plenty possible to adapt to supporting any local path, although there will be a bit of bookkeeping to sort out in that we'll need to convert the local file path to a package into a local file path to a module which we'll use in the replace and whose go.mod we'll read, and the module-path-qualified package name by which we'll actually import it.

[1] You can see this by creating a command, in a module, which does fmt.Println(runtime.Caller(0)), i.e. prints the source file from which it was compile. If you go run my.site/mod/path/to/package (or go run ./path/to/package/main.go from within the module, it will print$PWD/path/to/package/main.go. Elsewhere, it will print$GOPATH/src/my.site/mod/path/to/package/main.goif path mode is enabled (e.g.GO111MODULE=auto) and the package is available there andgo run` will error and not run anything if not.

cosmos72 commented 2 years ago

Implemented :) It "only" took one year (sweat).

Commit 3026d9a4ebc76e18ba5e9d1b4a1e407098da9d58 adds the ability to import local packages from a filesystem directory. Syntaxes are:

import (
     "."                             // imports package found in current directory
     ".."                            // imports package found in parent directory
     "./some/relative/path"          // "./"  means relative to current directory
     "../some/other/relative/path"   // "../" means relative to parent directory 
     "/some/absolute/path"           // "/"   means absolute
)
JeroenVerstraelen commented 1 year ago

This does not appear to work when the local package you are importing is also dependent on another local package.

Example when importing a local module with this go.mod file:

module github.com/JeroenVerstraelen/SanctusGo/gameServer
go 1.18
replace github.com/JeroenVerstraelen/SanctusGo/databases => ../databases
require (
    github.com/JeroenVerstraelen/SanctusGo/databases v0.0.0-00010101000000-000000000000
)
gomacro> import "."
// debug: running "go get github.com/JeroenVerstraelen/SanctusGo/gameServer" ...
go: downloading github.com/JeroenVerstraelen/SanctusGo/databases v0.0.0-00010101000000-000000000000
github.com/JeroenVerstraelen/SanctusGo/gameServer imports
        github.com/JeroenVerstraelen/SanctusGo/gameServer/shared imports
        github.com/JeroenVerstraelen/SanctusGo/databases: github.com/JeroenVerstraelen/SanctusGo/databases@v0.0.0-00010101000000-000000000000: invalid version: git ls-remote -q origin in /go/pkg/mod/cache/vcs/f8c7685f27da3fa5907dca5bde0c2025ec9dec4ee004e631a8138f85b4169aac: exit status 128:
        fatal: could not read Username for 'https://github.com': terminal prompts disabled

You can see that it still tries to get SanctusGo/databases from Github while importing Sanctus/gameServer. Instead I would like it to use the local directory.