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(pkg/doc): ensure mod packages are loaded correctly #1306

Closed
wants to merge 2 commits into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Oct 26, 2023

This is the "proper fix" for the issue encountered by @jefft0, @zivkovicmilos, @moul & others, fixed in make test by #1301.

Essentially, because of the way that gno doc handled directories with gno.mod, the cached version of gno.land/p/demo/avl had higher priority than its version in Gnomod.

I've added a txtar integration test to show the behaviour which fails on master and is fixed in this branch.

Fixes #1142.

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.

@thehowl thehowl requested a review from a team as a code owner October 26, 2023 19:38
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Oct 26, 2023
@github-actions github-actions bot removed the 🧾 package/realm Tag used for new Realms or Packages. label Oct 26, 2023
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (34d78b3) 47.92% compared to head (d825e2a) 47.90%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1306      +/-   ##
==========================================
- Coverage   47.92%   47.90%   -0.03%     
==========================================
  Files         372      372              
  Lines       62990    63007      +17     
==========================================
- Hits        30187    30181       -6     
- Misses      30344    30364      +20     
- Partials     2459     2462       +3     
Files Coverage Δ
gnovm/pkg/doc/dirs.go 86.20% <91.30%> (+0.21%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl thehowl marked this pull request as draft November 9, 2023 21:48
@thehowl
Copy link
Member Author

thehowl commented Nov 9, 2023

There are some questions I've raised myself about this implementation and in general about how to manage static analysis tools, gno.mod and dependencies. I started writing a wall of text here outlining the problem, coming up with a proposal, and deciding this needs another issue.

In any case, I've put this back into a draft while we clarify some issues. Expect an issue relating to this PR soon.

@thehowl
Copy link
Member Author

thehowl commented Mar 26, 2024

Current state: there is a crucial problem which has stung us time and time again in cmd/gno, which is that there is no current centralised way within the gno command to answer the question "If I have an import path x, which paths should I look to try to resolve this dependency and in which order should they be?".

There are also some related concerns on how, at the current state of gno development, we can manage having code which has the same import path but is on a different chain; ie. test3 vs. portal-loop. A proposal on how to solve this has been added in #1352.

All in all, this PR addresses an issue which likely should really be fixed with a v2 of #1299. I'll make another comment there, mark it as draft and close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

issue: gnovm/cmd/TestGnoDoc failed with incompatible gno packages from the earlier build
1 participant