-
Notifications
You must be signed in to change notification settings - Fork 373
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(gnovm): packages loader #2932
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2932 +/- ##
==========================================
- Coverage 63.37% 63.34% -0.04%
==========================================
Files 566 566
Lines 79490 79200 -290
==========================================
- Hits 50374 50166 -208
+ Misses 25727 25670 -57
+ Partials 3389 3364 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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]>
…ve download to own file Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
examples/gno.land/gno.mod
Outdated
@@ -0,0 +1 @@ | |||
module gno.land |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this file?
if we need it, please explain in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see #2904 (comment) and tell me what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best would be to have a gno.work
file at repo root to support multiple modules in a monorepo
But in that case we need to implement the gno work
commands
gnovm/tests/machine_test.go
Outdated
@@ -40,7 +40,7 @@ func TestMachineTestMemPackage(t *testing.T) { | |||
Name: tt.name, | |||
F: func(t2 *testing.T) { //nolint:thelper | |||
rootDir := filepath.Join("..", "..") | |||
store := TestStore(rootDir, "test", os.Stdin, os.Stdout, os.Stderr, ImportModeStdlibsOnly) | |||
store := TestStore(rootDir, "test", nil, os.Stdin, os.Stdout, os.Stderr, ImportModeStdlibsOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a variable that you set to nil, so we now what is this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 0123512
gnovm/pkg/packages/download.go
Outdated
func DownloadModule(pkgPath string, dst string) error { | ||
modFilePath := filepath.Join(dst, ModfileName) | ||
if _, err := os.Stat(modFilePath); os.IsNotExist(err) { | ||
fmt.Fprintln(os.Stderr, "gno: downloading", pkgPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually don't use os.StdXXX, but stdouterr from a struct (io something) which is passed when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in e648305
} | ||
} | ||
|
||
// write gno.mod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it depends, but to keep sanity, I think we do, see: #2904 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you first download a sub-module, for example gno.land/p/demo/json/eisel_lemire
it will create the $GNOHOME/pkg/mod/gno.land/p/demo/json
directory since it's a parent directory.
To know that the gno.land/p/demo/json
package needs to be downloaded, I'm checking for the presence of $GNOHOME/pkg/mod/gno.land/p/demo/json/gno.mod
gnovm/cmd/gno/test.go
Outdated
@@ -617,3 +592,21 @@ func shouldRun(filter filterMatch, path string) bool { | |||
ok, _ := filter.matches(elem, matchString) | |||
return ok | |||
} | |||
|
|||
// Adapted from https://yourbasic.org/golang/formatting-byte-size-to-human-readable-format/ | |||
func prettySize(nb int64) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why moving this from util?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conflict resolution fail, reverted in 53c5770
gnovm/pkg/packages/resolve.go
Outdated
Dir string `json:",omitempty"` | ||
ImportPath string `json:",omitempty"` | ||
Name string `json:",omitempty"` | ||
Root string `json:",omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments to explain the fields.
this one for instance, is it the "location of the package that can be either specified locally or automatically cached on the disk"? or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 457b0c9
Co-authored-by: Manfred Touron <[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]>
Thanks <3 |
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]>
WIP
Continuation of #2201
Taking #2904 into account
This implements a
Load
function with a similar interface to thepackages.Load
go functionContributors' checklist...
BREAKING CHANGE: xxx
message was included in the description