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

fix(mempkg): don't attempt to validate name of an empty package #1076

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 28, 2023

This PR adjusts the behavior of ReadMemPackage. It now skips the package name validation when no gno files are found in the package directory. This modification allows for the utilization of empty parent packages that have a native fallback. For instance, in the foo/bar scenario, where foo is an empty package but has a native implementation defined in imports.go, and bar is a standard package containing gno files. Previously, this would have triggered a panic on foo import due to the empty package name rather than appropriately using the native foo fallback.

While I haven't been able to introduce tests for this change, PR #1066, which incorporates the net/url package, would fail in the absence of this modification due to empty net dependencies.

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
  • Added new benchmarks to generated graphs, if any. More info here.

…no files)

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@gfanton gfanton self-assigned this Aug 28, 2023
@gfanton gfanton requested review from jaekwon, moul and a team as code owners August 28, 2023 09:01
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 28, 2023
@gfanton gfanton mentioned this pull request Aug 28, 2023
9 tasks
gno.land/cmd/gnoland/start.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/tests/imports.go Outdated Show resolved Hide resolved
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 💯

Left a few minor comments

gnovm/tests/package_test.go Outdated Show resolved Hide resolved
gnovm/tests/imports.go Show resolved Hide resolved
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
modify realm/package validate regexp to support stdlibs package that doesn't
have a domain part

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
This reverts commit 6ce9a3e.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@gfanton gfanton merged commit 6152030 into gnolang:master Sep 5, 2023
67 checks passed
@gfanton gfanton deleted the fix/empty-package branch September 5, 2023 15:20
@piux2
Copy link
Contributor

piux2 commented Sep 6, 2023

For instance, in the foo/bar scenario, where foo is an empty package but has a native implementation defined in imports.go, and bar is a standard package containing gno files. Previously, this would have triggered a panic on foo import due to the empty package name rather than appropriately using the native foo fallback.

The panic is expected behavior. We do not want to change it.

for foo/bar, only one package node created. pn.Name = bar pn.PkgPath = gnoland/xxx/foo/bar.
No package node is created for foo.

Try to inject native method at foo should should panic in stdlibs/stdlib.go ( for on chain ) and tests/import.go ( for off chain testing).

The native method can only be injected in a valid PackageNode.

Plus:

No need to check memPkg.IsEmpty(), this method is not needed.

Check the reason below

https://github.com/gnolang/gno/pull/1076/files/6ce9a3e72a8158f64ca321738239804356d573ff#r1316600063

moul added a commit that referenced this pull request Sep 22, 2023
Added the `net/url` package. This package will be beneficial for
manipulating URLs, for instance, parsing query parameters or for URL
muxing in realms.

depends:
- [x] #1076 
- [x] #1065 

<details><summary>Contributors' checklist...</summary>

- [X] 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
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
gfanton added a commit to gfanton/gno that referenced this pull request Nov 9, 2023
Added the `net/url` package. This package will be beneficial for
manipulating URLs, for instance, parsing query parameters or for URL
muxing in realms.

depends:
- [x] gnolang#1076 
- [x] gnolang#1065 

<details><summary>Contributors' checklist...</summary>

- [X] 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
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants