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

Store files in the proper location on Windows. #1656

Open
MicahZoltu opened this issue Sep 28, 2020 · 11 comments
Open

Store files in the proper location on Windows. #1656

MicahZoltu opened this issue Sep 28, 2020 · 11 comments
Labels
area/windows Windows effort/days Estimated to take multiple days, but less than a week exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up

Comments

@MicahZoltu
Copy link

  • OS: Windows
  • Version of IPFS Desktop: 0.12.2

Describe the bug
Installing IPFS Desktop on Windows results in folders in %USERPROFILE% as well as cache and DB files in the roaming profile.

To Reproduce
Steps to reproduce the behavior:

  1. Install IPFS Desktop on Windows
  2. Notice that you have a .ipfs and .ipfs-desktop folder in %USERPROFILE% and %APPDATA/IPFS Desktop contains cache files, databases, blob storage, etc.

Expected behavior
Cache files, file storage, databases, and per-machine configuration should all be stored in %LOCALAPPDATA%/ipfs/. Per user configuration (I'm not sure if IPFS has any of this, maybe the list of pinned files?) should be stored in %APPDATA%/ipfs/. Nothing should be stored in %USERPROFILE% (aka: %HOME%) unless the user *explicitly tells the app to do so such as via a save dialog box.

Additional context
Both Linux and OSX also have environment variables that instruct applications where the appropriate place for files is. I believe it is called XDG_CONFIG though I'm not personally familiar with it.

@MicahZoltu MicahZoltu added the need/triage Needs initial labeling and prioritization label Sep 28, 2020
@jessicaschilling jessicaschilling added need/analysis Needs further analysis before proceeding and removed need/triage Needs initial labeling and prioritization labels Sep 28, 2020
@jessicaschilling
Copy link
Contributor

Thanks, @MicahZoltu - we'll bring this up in our next weekly triage.

@lidel
Copy link
Member

lidel commented Oct 7, 2020

Did some spelunking on borrowed Windows 10 laptop and ($user is the name of my user):

  • %USERPROFILE% is C:/Windows/Users/$user/
  • %APPDATA% is C:/Windows/Users/$user/AppData/Roaming/
    • specific for every user and when joined to a windows domain (eg. in enterprise setting), synchronized over the network
  • %LOCALAPPDATA% is C:/Windows/Users/$user/AppData/Local/
    • same, but data stays local and does not get synchronized
  • %PROGRAMDATA% is C:\ProgramData (?)
    • data that applies to all users on the machine (let's not go into it right now, just to keep this analysis simpler)

For sure there are historical reasons for this, but this naming convention is pretty awkward.

Anyway, IPFS Desktop creates:

When installed only for current user

  • C:/Windows/Users/$user/.ipfs/ - go-ipfs config and repo
  • C:/Windows/Users/$user/.ipfs-desktop/ – config bridging GUI and CLI
  • C:/Windows/Users/$user/AppData/Roaming/IPFS Desktop/ – Electron app config and UI caches
  • C:/Windows/Users/$user/AppData/Local/Programs/IPFS Desktop/ – Electron and go-ipfs binaries

When installed for all users

afaik same, with one difference:

  • C:/Program Files/IPFS Desktop/ – Electron and go-ipfs binaries

iiuc the main issue here is with the first two, namely we have those ugly folders in %USERPROFILE% directory:

2020-10-07--15-29-15

My thoughts:

  • .ipfs:
    • This is a directory with configuration and data of go-ipfs daemon used by IPFS Desktop
    • There is no such thing as "per machine" and "per user" data - each user gets own daemon with unique configuration and repository, so everything is "per user"
    • By default go-ipfs assumes that IPFS_PATH=$HOME/.ipfs and on Windows $HOME seems to be translated to %USERPROFILE%
      • 🔧 We could override the default (set custom IPFS_PATH on Windows, when it is missing from env) and point at something like %LOCALAPPDATA%/go-ipfs/.ipfs
        • ❗ this requires extreme care to migrate existing setup without breaking or losing data of existing users
  • .ipfs-desktop
    • This seems to be only a subset of configuration, stuff related to CLI paths.
    • 🔧 We could safely remove this directory by moving it to the directory with Electron app config ($XDG_CONFIG/IPFS Desktop on Linux is $HOME/.config/IPFS Desktop, and $user/AppData/Roaming/IPFS Desktop on Windows)
    • Most of the users did not use PATH integration, nor customized binary/IPFS_PATH, so risk is low

@MicahZoltu does this sound sensible? Let me know if I missed some nuance.

I'm open for reviewing a PR if someone familiar with Windows creates one.

@lidel lidel added area/windows Windows need/author-input Needs input from the original author and removed need/analysis Needs further analysis before proceeding labels Oct 7, 2020
@lidel lidel removed their assignment Oct 7, 2020
@jessicaschilling jessicaschilling added the help wanted Seeking public contribution on this issue label Oct 7, 2020
@MicahZoltu
Copy link
Author

We could override the default (set custom IPFS_PATH on Windows, when it is missing from env) and point at something like %LOCALAPPDATA%/go-ipfs/.ipfs

A . prefix on Windows does not alter visibility and it causes some mild annoyances in Windows Explorer since you cannot set a folder to that name in some contexts. However, applications are free to do whatever they want within their subfolders of %LOCALAPPDATA% so if for some reason having a folder without a leading . is difficult then letting it stay I think is fine and still respects Windows folder hierarchy.

We could safely remove this directory by moving it to the directory with Electron app config ($XDG_CONFIG/IPFS Desktop on Linux is $HOME/.config/IPFS Desktop, and $user/AppData/Roaming/IPFS Desktop on Windows)

$user/AppData/Roaming should be %APPDATA%. It is configurable, and won't always be a subdirectory of %USERPROFILE% nor will it necessarily be a sibling of %LOCALAPPDATA% (in fact, it is common to have them on different drives). Does IPFS currently use %APPDATA%, %LOCALAPPDATA%, and %USERPROFILE% or is it manually constructing those paths? If it is manually constructing those paths then that should be fixed as well.

Something to be aware of is that Electron's default locations are bad and you should not assume that Electron will put data in the right place by default. Electron by default considers hundreds of megabytes of cache data to be "app config" and it stores it in a location that is network synced/backed up. Given this, I recommend moving the Electron stuff to %LOCALAPPDATA% since it can grow pretty large and is transient data (not actual user-configuration data).

In general, my advice is to use %LOCALAPPDATA% for everything as a default unless your application is specifically designed to support roaming/network synced configuration. I don't know if IPFS is currently designed for synced user configuration, though it would be pretty cool if it was. In particular, I would like it if my list of pinned files was synced and when I switched computers IPFS would automatically fetch pinned items and remove pins from items I unpinned on another computer. However, I would not want the actual pinned content to live in a roaming location, just the hashes.

@lidel lidel added exp/wizard Extensive knowledge (implications, ramifications) required effort/days Estimated to take multiple days, but less than a week P3 Low: Not priority right now kind/maintenance Work required to avoid breaking changes or harm to project's status quo and removed need/author-input Needs input from the original author labels Oct 9, 2020
@lidel
Copy link
Member

lidel commented Oct 9, 2020

Thank you @MicahZoltu, that is very useful feedback.

Did some digging and there are three separate topics here, each can be tackled separately:

On replacing %APPDATA% with %LOCALAPPDATA%

Main concern here is the cost of fixing Electron's shortcomings in our code over time.

We tried doing that many times in the past, and then when electron or electron-builder finally fixed the problem in upstream code, it broke our workarounds in terrible ways.

My opinionated advice here is to wait for upstream electron to fix the %APPDATA% vs %LOCALAPPDATA% problem.

Personally, I would not pick this up, as it is feels like a huge rabbit hole without any visible improvement to users, but if someone wants to tackle this, I'd be open to reviewing a PR that fixes paths for new Windows installations, as long it:

  • does not introduce a lot of new code
    • ideally, should be just a matter of detecting windows runtime and setting a flag/variable/config parameter (additional research is needed) in a way that won't break even when upstream electron changes their defaults at some point
  • does not migrate existing installations
    • if old IPFS Desktop/ exists in %APPDATA%, use it instead of creating new one in %LOCALAPPDATA%
    • this is because every migration code will break over time, and making sure we detect when it breaks requires expensive test suite to guard against regressions

On moving .ipfs-desktop from %USERPROFILE% to Electron app config dir

This can be tackled as a standalone PR:

  • refactor code and move configuration from $HOME/.ipfs-desktop/* to Electron app config dir (on Linux $XDG_CONFIG/IPFS Desktop/)
    • this cleanup will benefit all platforms

On moving .ipfs from %USERPROFILE% to %LOCALAPPDATA%

The %USERPROFILE% is the default picked by go-ipfs daemon when IPFS_PATH is missing.

Code responsible for this is in https://github.com/ipfs/go-ipfs/blob/v0.7.0/repo/fsrepo/misc.go#L13-L18 and https://github.com/mitchellh/go-homedir/blob/af06845cf3004701891bf4fdb884bfe4920b3727/homedir.go#L148-L166

I think ipfs-desktop could avoid polluting %USERPROFILE% if we:

  • detect windows runtime (IS_WIN)
    • if IPFS_PATH is set, use it as-is
    • else, if %USERPROFILE%/.ipfs exists, use it as IPFS_PATH to ensure existing installations do not break
    • else, set IPFS_PATH to be something explicit like %LOCALAPPDATA%/go-ipfs/IPFS_PATH, so new installations on Windows won't pollute %USERPROFILE%/
  • when all this is done, we still need to update relevant docs at https://docs.ipfs.io/how-to/move-ipfs-installation/#move-installation

ps. The problem will remain if user runs go-ipfs without Electron wrapper. The default IPFS_PATH is a decision go-ipfs daemon does, so perhaps this should be also fixed upstream in BestKnownPath too.

@MicahZoltu
Copy link
Author

MicahZoltu commented Oct 10, 2020

I submitted an issue to get go-homedir library fixed: mitchellh/go-homedir#30

I believe there are other similar libraries that better support different data types so if the author of go-homedir is resistant to this change, perhaps switching go-ipfs to a different library would be the right course of action long term. Either way, migration will need to be handled still so the library getting fixed isn't the final solution.


Main concern here is the cost of fixing Electron's shortcomings in our code over time.

My opinionated advice here is to wait for upstream electron to fix the %APPDATA% vs %LOCALAPPDATA% problem.

My concern with this strategy is that this issue has been open with Electron for almost 4 years now, and until they fix it the default of Electron is to be a bad citizen. This means that IPFS Desktop will continue to be a bad citizen indefinitely, since there is no light at the end of the tunnel for Electron fixing the issue.

That being said, I understand your position and appreciate you clearly laying out why you are weakly opposed to it. I certainly don't envy the maintainers of IPFS Desktop being put in this situation.


All in all, I agree with all 3 of your suggested paths forward. They are prudent and get things to a better place eventually without risking too much code complexity or breakage along the way.

@MicahZoltu
Copy link
Author

As an intermediate step, can we get a configuration option added that will allow me to tell IPFS Desktop where to write? It seems that Electron has no intention of fixing this (going on 5 years open bug with no activity) and IPFS is probably the worst offender if you actively use it and pin with it (I'm looking at 2GB of "cache data" in my .ipfs folder right now).

@MicahZoltu
Copy link
Author

Note: I tried setting %USERPROFILE%\.ipfs-desktop\IPFS_PATH to point somewhere else and I also tried setting an environment variable for IPFS_PATH. In the first case, it just overwrote my IPFS_PATH file on startup. In the second case, it didn't do anything.

@MicahZoltu
Copy link
Author

It seems that after first launch the path is stored in %APPDATA%/IPFS Desktop/config.json and reconstituted from there. Unfortunately, there doesn't appear to be any way to tell it to not write the .ipfs-desktop folder to %USERPROFILE%.

@lidel
Copy link
Member

lidel commented May 4, 2021

Thank you @MicahZoltu, this is useful feedback.
IIUC someone familiar with Windows may be able to fix this for new users with a set of surgical tweaks (only on Windows platform):

  1. Detect when IPFS_PATH is NOT set, and proactively set it to a better location in env with which go-ipfs binary is called – this will solve the .ipfs placement in a way that won't break in the future
  2. Use the same for .ipfs-desktop – I believe on Windows we only use it in:
    • ./src/daemon/index.js
    • ./src/daemon/daemon.js
    • ./src/ipfs-on-path/scripts/bin-win/ipfs.cmd (deprecated experiment, but some users still run it)

We don't have bandwidth for doing going this extra mile on top of Electron defaults,
but if someone really cares, PRs are welcome 🙏

@MicahZoltu
Copy link
Author

I suspect that Linux users would also like to see this fixed to respect XDG_CONFIG paths, and I think OSX similarly has some feature for defining where cache files go. The solution will likely be a bit different for each platform, so it does seem like fixing each separately may make the most sense (especially if you are looking to outsource the work).

@lidel
Copy link
Member

lidel commented Jul 2, 2021

Extracting action plan for most valuable fix for Windows users (#1862):

If a user installs IPFS Desktop on Windows, it will create multiple folders:

%AppData%\Roaming\IPFS Desktop
%USERPROFILE%.ipfs
%USERPROFILE%.ipfs-desktop

That's pretty unintuitive where what data will be stored and why.

Describe the solution you'd like
Let IPFS Desktop create a single folder in %AppData% and move the rest of the folders below it.

Only caveat here is to do some magic similar to #1656 (comment) (need someone to check if that still makes sense) to ensure change applies to new installs and does not break repos of existing users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/windows Windows effort/days Estimated to take multiple days, but less than a week exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up
Projects
No open projects
Status: Needs Grooming
Development

No branches or pull requests

3 participants