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

feat: add GNOROOT environment variables #1014

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 1, 2023

This small PR adds the ability to obtain the root directory from the GNOROOT environment variable. If this variable is not set, the root directory is then guessed using the go list command.
This enable the user to manually set gno root dir globally and not bother setting the root-dir parameter every time he need need it

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.

@gfanton gfanton requested a review from a team as a code owner August 1, 2023 17:58
@gfanton gfanton self-assigned this Aug 1, 2023
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 1, 2023
@moul
Copy link
Member

moul commented Aug 1, 2023

Thank you for your first contribution!

Your PR is related to #682. I recommend discussing with the @harry-hov, the other contributor to merge your changes efficiently. You can propose a plan for merging PRs, their order, or cherry-picking specific changes.

I'll assign each other as reviewers for better collaboration.

Adds the ability to obtain the root directory from the GNOROOT environment variable. If this variable is not set, the root directory is then guessed using the go list command.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@gfanton gfanton changed the title feat: add GNO_ROOT_DIR environment variables feat: add GNOROOT environment variables Aug 2, 2023
@harry-hov harry-hov self-requested a review August 3, 2023 10:25
Comment on lines +113 to 120
// if GNOROOT is not set, try to guess the root directory using the `go list` command.
cmd := exec.Command("go", "list", "-m", "-mod=mod", "-f", "{{.Dir}}", "github.com/gnolang/gno")
out, err := cmd.CombinedOutput()
if err != nil {
log.Fatal("can't guess --root-dir, please fill it manually.")
log.Fatal("can't guess --root-dir, please fill it manually or define the GNOROOT environment variable globally.")
}
rootDir := strings.TrimSpace(string(out))
return rootDir
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if GNOROOT is not set, try to guess the root directory using the `go list` command.
cmd := exec.Command("go", "list", "-m", "-mod=mod", "-f", "{{.Dir}}", "github.com/gnolang/gno")
out, err := cmd.CombinedOutput()
if err != nil {
log.Fatal("can't guess --root-dir, please fill it manually.")
log.Fatal("can't guess --root-dir, please fill it manually or define the GNOROOT environment variable globally.")
}
rootDir := strings.TrimSpace(string(out))
return rootDir

I really want to get rid of this part.

Either use --rootdir or set GNOROOT. no guessing anymore :)

this will affect many commands/functionalities. Since it's a breaking change. If we all agree on this, it can be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest opening up a separate PR for this change as it has ramifications for existing user functionality

Copy link
Contributor

@harry-hov harry-hov left a comment

Choose a reason for hiding this comment

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

LGTM 💯

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.

Thank you for the contribution 🙏

Looks good 🚀

@moul
Copy link
Member

moul commented Aug 6, 2023

Let's make sure GNOROOT points to a structured directory, not the clone directory, just like GOROOT. For now, we can assume the repository is already "compatible" with GNOROOT to avoid migration headaches.

To help developers, we must provide detailed documentation for all environment variables, default values/behaviors. And to keep things clear and manageable, we should limit the number of environment variables.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

I'll merge the changes now.

Before updating the documentation, let's wait for the new behavior by @harry-hov. It won't expect GNOROOT to have an examples folder anymore. Instead, it'll use a folder with cached packages, which will download missing dependencies and could get automatically filled when installing the gno binary or running it for the first time.

@moul moul merged commit 5d3be42 into gnolang:master Aug 6, 2023
52 checks passed
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 16, 2023
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
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants