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

eth, node: use APPDATA env to support cygwin/msys correctly #17786

Merged
merged 4 commits into from
Feb 19, 2019

Conversation

hackmod
Copy link
Contributor

@hackmod hackmod commented Sep 29, 2018

since windows XP $APPDATA (%APPDATA%) is used.
Please see also

for normal windows native environment, %HOME%(C:\Users\foobar) + \AppData\Roaming is correctly interpreted as %APPDATA%, but for cygwin/msys2 environment or powershell, %HOME% is /home/foobar/ and %HOME% + \AppData\Roaming is not %APPDATA% anymore.

Simply using %APPDATA% fix this issue.

@fjl
Copy link
Contributor

fjl commented Oct 15, 2018

Sorry for not following up with this last two weeks. I support this change, but fear that people will complain because geth doesn't find their old datadir anymore. Could you add a check to see whether there is a datadir at the old location? We already have backwards-compatibility checks like that for files inside datadir here.

While we're at it, there is an old issue about this: #2239. It says we shouldn't use AppData\Roaming because that might be located on a network drive.

@hackmod
Copy link
Contributor Author

hackmod commented Oct 21, 2018

we can use %LOCALAPPDATA% env variable for AppData/Local, but %LOCALAPPDATA% env is only available above WIN Vista.

  • for Ethash cache dir case - %LOCALAPPDATA% is more applicable.
  • other %APPDATA% cases - since Win XP, %APPDATA% env is supported and using %APPDATA% is more reasonable but need to check previously saved data like as you already pointed out isOldGethResource related method. (or we can ignore for XP case)

@hackmod
Copy link
Contributor Author

hackmod commented Oct 21, 2018

now, we can see %LOCALAPPDATA% is the right place for datadir.

how about this?

@@ -59,12 +62,28 @@ func DefaultDataDir() string {
                if runtime.GOOS == "darwin" {
                        return filepath.Join(home, "Library", "Ethereum")
                } else if runtime.GOOS == "windows" {
+                       dataDir := filepath.Join(home, "AppData", "Roaming", "Ethereum")
+                       if common.FileExist(filepath.Join(dataDir, "chaindata")) {
+                               log.Warn(fmt.Sprintf("XXX"))
+                               return dataDir
+                       }
                        appdata := os.Getenv("APPDATA")
                        if appdata != "" {
-                               return filepath.Join(appdata, "Ethereum")
+                               dataDir = filepath.Join(appdata, "Ethereum")
+                       } else {
+                               dataDir = filepath.Join(home, "AppData", "Roaming", "Ethereum")
+                       }
+                       if common.FileExist(filepath.Join(dataDir, "chaindata")) {
+                               log.Warn(fmt.Sprintf("XXX"))
+                               return dataDir
+                       }
+                       localappdata := os.Getenv("LOCALAPPDATA")
+                       if localappdata != "" {
+                               dataDir = filepath.Join(localappdata, "Ethereum")
                        } else {
-                               return filepath.Join(home, "AppData", "Roaming", "Ethereum")
+                               dataDir = filepath.Join(home, "AppData", "Local", "Ethereum")
                        }
+                       return dataDir
                } else {
                        return filepath.Join(home, ".ethereum")
                }

it looks somewhat ugly, but happy for old users and less issue for newcomers.

@holiman
Copy link
Contributor

holiman commented Jan 15, 2019

Changing this also involves Mist, which (at least historically) has sometimes interacted directly with the keystore folder (although I don't remember exactly under what circumstances, maybe during import). cc @evertonfraga what would happen if we move the keystore ?

@fjl fjl added this to the 1.9.0 milestone Jan 25, 2019
@fjl fjl self-assigned this Jan 25, 2019
@fjl fjl changed the title node: use APPDATA env to support cygwin/msys correctly eth, node: use APPDATA env to support cygwin/msys correctly Feb 19, 2019
@fjl fjl merged commit f7f6a46 into ethereum:master Feb 19, 2019
@hackmod hackmod deleted the win32-appdata branch February 21, 2019 16:06
denis-papyrus pushed a commit to papyrusglobal/go-ethereum that referenced this pull request Mar 12, 2019
…#17786)

This changes default location of the data directory to use the LOCALAPPDATA
environment variable, resolving issues with remote home directories an improving
compatibility with Cygwin.

Fixes ethereum#2239 
Fixes ethereum#2237 
Fixes ethereum#16437
kiku-jw pushed a commit to kiku-jw/go-ethereum that referenced this pull request Mar 29, 2019
…#17786)

This changes default location of the data directory to use the LOCALAPPDATA
environment variable, resolving issues with remote home directories an improving
compatibility with Cygwin.

Fixes ethereum#2239 
Fixes ethereum#2237 
Fixes ethereum#16437
hackmod added a commit to Ethersocial/go-ethersocial that referenced this pull request Apr 14, 2019
…#17786)

This changes default location of the data directory to use the LOCALAPPDATA
environment variable, resolving issues with remote home directories an improving
compatibility with Cygwin.

Fixes ethereum#2239
Fixes ethereum#2237
Fixes ethereum#16437
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants