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!: use os.UserConfigDir() if $GNO_HOME not set #673

Closed
wants to merge 6 commits into from

Conversation

grepsuzette
Copy link
Contributor

@grepsuzette grepsuzette commented Mar 29, 2023

Description

In #652 Moul talked about supporting *nix stardard XDG variables.
I'm also a fan of this.

Propose going for $XDG_CONFIG_HOME.
So the order would become:

  1. $GNO_HOME
  2. $XDG_CONFIG_HOME/gno (usually this variable is $HOME/.config)
  3. .gno in the home directory

Fixes #655

How has this been tested?

I just introduced a panic:

if hd != "" {
        panic(fmt.Sprintf("%s/gno", hd))
        return fmt.Sprintf("%s/gno", hd)
    }

and it returned /home/bob/.config/gno

@grepsuzette grepsuzette requested a review from a team as a code owner March 29, 2023 02:34
@thehowl
Copy link
Member

thehowl commented Mar 29, 2023

#655 - let's use os.UserConfigDir instead of XDG_CONFIG_HOME?

@moul
Copy link
Member

moul commented Mar 29, 2023

I also prefer os.UserConfigDir over raw $XDG_.

@grepsuzette
Copy link
Contributor Author

grepsuzette commented Mar 29, 2023

Yes makes sense.

Tested with the same method
and it returned /home/bob/.config/gno when $GNO_HOME not set.

@grepsuzette grepsuzette changed the title chore: fallback to $XDG_CONFIG_HOME/gno if $GNO_HOME not set chore: use os.UserConfigDir() if $GNO_HOME not set Mar 29, 2023
@thehowl thehowl changed the title chore: use os.UserConfigDir() if $GNO_HOME not set feat!: use os.UserConfigDir() if $GNO_HOME not set Mar 30, 2023
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Some comments.

One note for you - your PR will likely be merged after #585. Seeing as that's ready, we're trying to merge as few crucial and small things as possible in order to avoid endlessly solving merge conflictts on that branch - although merging it will probably create a conflict on this one.


// look for dir in home directory.
hd, err := os.UserHomeDir()
hd, err = os.UserConfigDir()
Copy link
Member

Choose a reason for hiding this comment

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

I was looking through the go source, and it looks like the behaviour on non-windows is always a Getenv("HOME") for both UserConfigDir and UserHomeDir. Which means that realistically the result of the function is either hd + "/gno" or a panic. I think we can change this so that

hd, err = ...
if err != nil // panic
return filepath.Join(hd, "gno")

also, change variable name to cd? (h doesn't make sense anymore)

Copy link
Contributor Author

@grepsuzette grepsuzette Mar 30, 2023

Choose a reason for hiding this comment

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

they don't use xdg? why did they make an API in that case 🙄
ok I will look that up. Thanks for the notice!

Edit: but no, I tested it, it returns /home/bob/.config/gno. Or you meant Windows instead of non-windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look the doc https://docs.studygolang.com/pkg/os/#UserConfigDir
but it seems ok to me:

func UserConfigDir() (string, error)

UserConfigDir returns the default root directory to use for user-specific configuration data. Users should create their own application-specific subdirectory within this one and use that.

On Unix systems, it returns $XDG_CONFIG_HOME as specified by https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html if non-empty, else $HOME/.config. On Darwin, it returns $HOME/Library/Application Support. On Windows, it returns %AppData%. On Plan 9, it returns $home/lib.

If the location cannot be determined (for example, $HOME is not defined), then it will return an error.

And

func UserHomeDir 1.12

func UserHomeDir() (string, error)

UserHomeDir returns the current user's home directory.

On Unix, including macOS, it returns the $HOME environment variable. On Windows, it returns %USERPROFILE%. On Plan 9, it returns the $home environment variable.

pkgs/crypto/keys/client/common.go Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This is more of an addendum to what we were saying on #655, but while @moul said that we don't need to ensure backwards compatibility, because in this case we're changing the behaviour of gnokey where the keys are stored (so something literally everyone who has tinkered with the project will have done), I think it makes sense here to add steps to transition things properly.

I think we can either:

  1. Implicitly move ~/.gno to join(configDir, "gno")
  2. Show a warning if we can stat ~/.gno without an error like: The default value for --home / GNO_HOME has been changed to <newConfigDir>, and you still have a directory in <homeDir>/.gno . We suggest to either set in your environment GNO_HOME to <homeDir>/.gno, or move it to the new directory.

(I'm much more of a fan of the first -- if we want to be transparent, adding a log message saying we've moved the dirs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more of an addendum to what we were saying on #655, but while @moul said that we don't need to ensure backwards compatibility, because in this case we're changing the behaviour of gnokey where the keys are stored (so something literally everyone who has tinkered with the project will have done), I think it makes sense here to add steps to transition things properly.

I think we can either:

1. Implicitly move `~/.gno` to `join(configDir, "gno")`

2. Show a warning if we can stat `~/.gno` without an error like: `The default value for --home / GNO_HOME has been changed to <newConfigDir>, and you still have a directory in <homeDir>/.gno . We suggest to either set in your environment GNO_HOME to <homeDir>/.gno, or move it to the new directory`.

(I'm much more of a fan of the first -- if we want to be transparent, adding a log message saying we've moved the dirs)

I don't think we should move a hidden directory such as ~/.gno somewhere else.
There is nothing to add here in my opinion, not even a message.
Complicating the TUI UX for no reason, and introducing potential bugs, while people may have backup procedures already in place for ~/.gno is not a good idea.

Because remember, it will just continue to work for existing users ($HOME/.gno being the last resort fallback).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Regarding my comment about filepath, I took a look at the usages of it through the repo.
It's probably better to keep using it for consistency, as there are many other usages of filepath besides the separator thing.)

@grepsuzette
Copy link
Contributor Author

Some comments.

One note for you - your PR will likely be merged after #585. Seeing as that's ready, we're trying to merge as few crucial and small things as possible in order to avoid endlessly solving merge conflictts on that branch - although merging it will probably create a conflict on this one.

Thanks for the note.
It's fine if there's a conflict, it will help me familiarize with the new repo structure :)

@grepsuzette
Copy link
Contributor Author

@thehowl I tried but think I can not solve the conflict without losing the git blame info. So I will open a new PR, closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

change default directory for $GNO_HOME on gnokey
3 participants