feat(gnomod): forbid require and find dependencies without it #3123
Conversation
Signed-off-by: Norman Meier <[email protected]>
…ould be reverted) Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
zivkovicmilos
added
the
breaking change
| res, err := gnoimports.PackageImports(root) | ||
| _ = err | ||
|
|
||
| entries, err := os.ReadDir(root) |
There was a problem hiding this comment.
Shouldn't this go before the PackageImports call?
There was a problem hiding this comment.
since we sort, it doesn't matter
There was a problem hiding this comment.
Why don't we do error handling here?
gnoimports.PackageImports does the same ReadDir, but we swallow errors from both calls?
There was a problem hiding this comment.
Better proposal: let's use fs.WalkDir instead of a recursive fn
gnovm/pkg/gnopkgfetch/gnopkgfetch.go
Outdated
| // we need to know that gno.land/r/foo is not downloaded. | ||
| // We do this by checking for the presence of gno.land/r/foo/gno.mod | ||
| if err := os.WriteFile(modFilePath, []byte("module "+pkgPath+"\n"), 0o644); err != nil { | ||
| return fmt.Errorf("failed to write modfile at %q: %w", modFilePath, err) |
There was a problem hiding this comment.
There's no cleanup if this fails, this is why I suggest you break up this mega func
There was a problem hiding this comment.
As was the case with the previous implementation
I agree with you, we also miss a filelock to avoid conflict between multiple gno command instances. I intend to improve all this in the subsequent PRs towards the unified package loader but I feel it's out of scope of this one
There was a problem hiding this comment.
As was the case with the previous implementation
💀
| @@ -0,0 +1,99 @@ | |||
| package gnopkgfetch | |||
There was a problem hiding this comment.
This is not a test file, so it's compiled into the binary. Please change it 🙏
There was a problem hiding this comment.
I'm not sure how to do it any other way, pls see #3123 (comment)
There was a problem hiding this comment.
Can't you drop it now, after this fix you mentioned?
| examplesRoot string | ||
| } | ||
|
|
||
| func (m *examplesMockClient) SendRequest(ctx context.Context, request types.RPCRequest) (*types.RPCResponse, error) { |
There was a problem hiding this comment.
Can we clean up this mock a bit? 🙏
There was a problem hiding this comment.
I'll clean up if you validate the new way it's injected :) otherwise I'll probably have to scrap it
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| res := ctypes.ResultABCIQuery{} | ||
|
|
||
| finfo, err := os.Stat(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
| } | ||
|
|
||
| if finfo.IsDir() { | ||
| entries, err := os.ReadDir(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
| } | ||
| res.Response.Data = []byte(strings.Join(files, "\n")) | ||
| } else { | ||
| content, err := os.ReadFile(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
|
|
||
| res := ctypes.ResultABCIQuery{} | ||
|
|
||
| finfo, err := os.Stat(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
| } | ||
|
|
||
| if finfo.IsDir() { | ||
| entries, err := os.ReadDir(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
| } | ||
| res.Response.Data = []byte(strings.Join(files, "\n")) | ||
| } else { | ||
| content, err := os.ReadFile(target) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
There was a problem hiding this comment.
I think the changes are good for now. Thank you for applying the fixes 🙏
Please see my leftover comments, so we can avoid having gnopkgfetch_testing compiled 🙏
When we fix this, I think we can move forward with the PR
Pinging @thehowl for a second screen
| @@ -105,7 +105,7 @@ func Parse(file string, data []byte) (*File, error) { | |||
| Err: fmt.Errorf("unknown block type: %s", strings.Join(x.Token, " ")), | |||
| }) | |||
| continue | |||
| case "module", "require", "replace": | |||
There was a problem hiding this comment.
I don't know if it should go here or elsewhere, but gno mod tidy should be capable of removing require blocks. This way gno mod tidy can automatically fix other repos
There was a problem hiding this comment.
Why add this extra complexity and maintenance burden when we can just recommended a well crafted sed one-liner?
| @@ -0,0 +1,72 @@ | |||
| package gnoimports | |||
There was a problem hiding this comment.
Definitely shouldn't be called gnoimports; and thinking that this should possibly be together with the package loader logic eventually. Can we use packages for now (inspired by x/tools/go/packages), with the following godoc?
// Package packages provides utility functions to statically analyze Gno MemPackages.There was a problem hiding this comment.
The go x/tools/go/packages is a wrapper around go list command invocations.
As stated in it's doc, it's responsibility is: loads Go packages for inspection and analysis.
I dived deep into the "loads Go packages" part code and it involves a lot more things than utility functions to statically analyze packages.
It has to pass by the filesystem since it's also responsible for downloading missing packages so in our case it couldn't operate on only MemPackages.
I think we should keep the gnoimports name with this doc:
// Package gnoimports provides utility functions to get the list of imports from a gno file or package
Added the doc file in d90f89a
| "strings" | ||
| ) | ||
|
|
||
| // PackageImports returns the list of gno imports from a given path. |
There was a problem hiding this comment.
With the previous proposal, I'd say to call this function Imports instead (usage: packages.Imports), and to accept a MemPackage rather than a path to the filesystem.
There was a problem hiding this comment.
No call site have a MemPackage handy, I could refacto things but it seems out of scope and not really an improvement
gnovm/pkg/gnoimports/imports.go
Outdated
| } | ||
|
|
||
| // FileImports returns the list of gno imports in a given file. | ||
| func FileImports(fname string) ([]string, error) { |
There was a problem hiding this comment.
This should process a filename + body rather than a filename.
Can you change pkg/test.LoadImports to use this function?
There was a problem hiding this comment.
I'm not sure it's better this way though
gnovm/pkg/gnoimports/imports.go
Outdated
| } | ||
| res := make([]string, len(f.Imports)) | ||
| for i, im := range f.Imports { | ||
| importPath := strings.TrimPrefix(strings.TrimSuffix(im.Path.Value, `"`), `"`) |
| func fetchPackage(io commands.IO, pkgPath string, dst string) error { | ||
| modFilePath := filepath.Join(dst, "gno.mod") | ||
|
|
||
| if _, err := os.Stat(modFilePath); err == nil { |
There was a problem hiding this comment.
Should be a way to force download (ie. -u)
There was a problem hiding this comment.
Seems out of scope but I can do it in this PR if you really want
| @@ -0,0 +1,164 @@ | |||
| package gnopkgfetch | |||
There was a problem hiding this comment.
I'd propose to have this package with a simpler API: Download(pkgPath, dst string). The code that walks the directory and loads the imports, and detects whether they're downloaded should go in cmd/gno; so we also restrict the output from happening within there.
This can then be called pkgdownload; likely this should eventually sit as a subpackage of the modules loader (maybe add this as a note comment in the package-level godoc, which you should also write ;) )
| "strings" | ||
| "unicode" | ||
|
|
||
| "github.com/gnolang/gno/gno.land/pkg/sdk/vm" |
There was a problem hiding this comment.
Important: please remove this dependency
(I'm tempted to think that eventually, we could have gnoweb serve a tarball of the package for downloaded; and avoid any tm2 client here; but this is for another time.)
| if rpcURL == "gno-examples" { | ||
| examplesDir := filepath.Join(gnoenv.RootDir(), "examples") | ||
| return tm2client.NewRPCClient(&examplesMockClient{examplesRoot: examplesDir}), nil | ||
| } |
There was a problem hiding this comment.
Still unclear how this works and whether it is a good idea.
| Draft bool // whether the package is a draft | ||
| Dir string // absolute path to package dir | ||
| Name string // package name | ||
| Imports []string |
There was a problem hiding this comment.
There should be a comment on the fact that this is transitional as well
There was a problem hiding this comment.
It's only direct imports actually
address: gnolang#3123 Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
This reverts commit a12d12a.
Signed-off-by: Norman Meier <[email protected]>
A step towards the importer package (#2932) and future of
gno.mod(#2904)requirestatement support fromgno.mod.gnofilesimportstatements to find dependenciesgnovm/pkg/gnopkgfetchGNO_PKG_HOSTSenv variable that takesdomain=rpcURLcoma-separated pairs to override pkg fetching behaviorgnovm/pkg/gnoimportsI decided to do this first to avoid having multiple ways to resolve dependencies lying around in the codebase and causing confusion in subsequent steps
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description