feat(gnomod): forbid require and find dependencies without it #3123

n0izn0iz · 2024-11-14T07:36:12Z

A step towards the importer package (#2932) and future of gno.mod (#2904)

BREAKING CHANGE: remove require statement support from gno.mod
use .gno files import statements to find dependencies
extract package download routines in gnovm/pkg/gnopkgfetch
add a GNO_PKG_HOSTS env variable that takes domain=rpcURL coma-separated pairs to override pkg fetching behavior
add and use a mocked rpc client that "fetch" packages from the examples dir
extract imports gathering utils in gnovm/pkg/gnoimports

I 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...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

Signed-off-by: Norman Meier <[email protected]>

…ould be reverted) Signed-off-by: Norman Meier <[email protected]>

Signed-off-by: Norman Meier <[email protected]>

…e-require

Signed-off-by: Norman Meier <[email protected]>

n0izn0iz · 2024-11-14T18:47:28Z

seems some tests are failing after merging master, my bad, will fix asap

Signed-off-by: Norman Meier <[email protected]>

n0izn0iz · 2024-11-15T07:11:00Z

fixed

Signed-off-by: Norman Meier <[email protected]>

…e-require

zivkovicmilos

Left some comments, that you for the require removal 🙏

If this is the route we wanna go with, the PR I think accomplishes what it needs to accomplish.
Please check some comments for testing code, it's the only thing I believe we need to modify

zivkovicmilos · 2024-11-22T08:48:21Z

gnovm/pkg/doc/dirs.go

+	res, err := gnoimports.PackageImports(root)
+	_ = err
+
+	entries, err := os.ReadDir(root)


Shouldn't this go before the PackageImports call?

since we sort, it doesn't matter

zivkovicmilos · 2024-11-22T08:50:32Z

gnovm/pkg/doc/dirs.go

+		sub := packageImportsRecursive(filepath.Join(root, entry.Name()))
+
+		for _, imp := range sub {
+			if !slices.Contains(res, imp) {


I was just about to ask how we filter duplicates

This is probably alright, I'd be curious to see how performant this is in comparison to a map lookup

map is faster for this after around 1K import statements: https://gist.github.com/n0izn0iz/3551867b864ef9ca155230a7ea8bb120
do you want me to replace with a map lookup?

zivkovicmilos · 2024-11-22T08:50:50Z

gnovm/pkg/doc/dirs.go

+		}
+	}
+
+	sort.Strings(res)


Why do they need to be sorted?

This code replaces getting the require list of a module and go mod tidy sorts imports. It might not be needed in this case though

zivkovicmilos · 2024-11-22T08:51:51Z

gnovm/pkg/gnoimports/imports_test.go

+	require.Equal(t, expected, imports)
+}
+
+func createTmpDir(t *testing.T) (string, func()) {


Why not simply use t.TempDir()?

This was moved from gnovm/cmd/gno/mod_test.go

Refactored in ed89ff6

zivkovicmilos · 2024-11-22T08:53:06Z

gnovm/pkg/gnoimports/imports.go

+	seen := make(map[string]struct{})
+	for _, e := range entries {
+		filename := e.Name()
+		if !strings.HasSuffix(filename, ".gno") {


Do we have these values as constants anywhere?
I swear we repeat this string literal 9000 times in the codebase

Doesn't look like it, I wanted to create a gnofiles package with these constants and related utils but it felt out of scope

zivkovicmilos · 2024-11-22T09:20:27Z

gnovm/pkg/gnopkgfetch/gnopkgfetch.go

+}
+
+// not using gno client due to cyclic dep
+func qfile(tmClient tm2client.Client, pkgPath string) ([]byte, error) {


Don't we have a client method for something like this?

doesn't seem so, gnoweb does something very similar here

btw the comment "not using gno client due to cyclic dep" was some confusion by me since gnoclient is a pkg dedicated to mutations, not queries

zivkovicmilos · 2024-11-22T09:21:12Z

gnovm/pkg/gnopkgfetch/gnopkgfetch.go

+	// 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's no cleanup if this fails, this is why I suggest you break up this mega func

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

zivkovicmilos · 2024-11-22T09:21:36Z

gnovm/pkg/gnopkgfetch/gnopkgfetch_testing.go

@@ -0,0 +1,99 @@
+package gnopkgfetch


This is not a test file, so it's compiled into the binary. Please change it 🙏

I'm not sure how to do it any other way, pls see #3123 (comment)

zivkovicmilos · 2024-11-22T09:22:11Z

gnovm/pkg/gnopkgfetch/gnopkgfetch_testing.go

+		t.Fatalf("failed to get source path")
+	}
+	examplesDir := filepath.Join(filepath.Dir(filename), "..", "..", "..", "examples")
+	oldClient := fetchClient


:(

We can do better 🙏

I'm open to ideas

zivkovicmilos · 2024-11-22T09:23:24Z

gnovm/pkg/gnopkgfetch/gnopkgfetch_testing.go

+	examplesRoot string
+}
+
+func (m *examplesMockClient) SendRequest(ctx context.Context, request types.RPCRequest) (*types.RPCResponse, error) {


Can we clean up this mock a bit? 🙏

It's very hard to read

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]>