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

Use standard directories #1805

Merged
merged 3 commits into from
Jan 17, 2025
Merged

Use standard directories #1805

merged 3 commits into from
Jan 17, 2025

Conversation

peterjan
Copy link
Member

Uses standard locations for application data instead of the current directory. This brings renterd in line with other system services and makes it easier to manage application data.

Linux, FreeBSD, OpenBSD

  • Configuration: /etc/walletd/walletd.yml
  • Data directory: /var/lib/walletd

macOS

  • Configuration: ~/Library/Application Support/walletd.yml
  • Data directory: ~/Library/Application Support/walletd

Windows

  • Configuration: %APPDATA%\SiaFoundation\walletd.yml
  • Data directory: %APPDATA%\SiaFoundation\walletd

Docker

  • Configuration: /data/walletd.yml
  • Data directory: /data

@peterjan peterjan self-assigned this Jan 15, 2025
@peterjan
Copy link
Member Author

Keeping it in DRAFT until I've tested it but can already be reviewed 👀

@peterjan peterjan force-pushed the pj/use-standard-directories branch from 585f459 to 22b0ef5 Compare January 16, 2025 10:08
@peterjan peterjan force-pushed the pj/use-standard-directories branch from 22b0ef5 to 087e257 Compare January 16, 2025 10:14
@peterjan peterjan marked this pull request as ready for review January 16, 2025 10:14
@peterjan
Copy link
Member Author

@ChrisSchinnerl @n8maninger the renterd desktop app uses the following folder to store renterd data /Users/peterjan/Library/Application Support/renterd/data while these changes will nudge me towards using /Users/peterjan/Library/Application Support/renterd/... I think ideally those should match and I'm suspecting our other apps will run into the same situation.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Lgtm so far. Haven't tested it locally yet but will do.

fmt.Println("This directory should be on a fast, reliable storage device, preferably an SSD.")
fmt.Println("")

_, existsErr := os.Stat(filepath.Join(dir, "consensus"))
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably do io.ReadDir and then check if that directory contains anything rather than checking for a specific folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this open so Nate sees it. Perhaps he wants to change it on walletd and hostd as well.

Copy link
Member

@n8maninger n8maninger Jan 16, 2025

Choose a reason for hiding this comment

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

Checking for anything when it's potentially PWD is not great. That's why I checked specifically for the host/wallet database. Everything else can be rebuilt.

Copy link
Member Author

@peterjan peterjan Jan 16, 2025

Choose a reason for hiding this comment

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

I've reverted it to check for consensus. I figured it was fine because we merely warn the user. Up for other alternatives but we don't really have the db options since we do SQLite and MySQL.

cmd/renterd/node.go Outdated Show resolved Hide resolved
cmd/renterd/config.go Outdated Show resolved Hide resolved
Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Approving but I'd still like to get some feedback from @n8maninger and @alexfreska first before merging this. On the discrepancy between this and the desktop apps.

@n8maninger
Copy link
Member

Approving but I'd still like to get some feedback from @n8maninger and @alexfreska first before merging this. On the discrepancy between this and the desktop apps.

We can handle the discrepancy in the desktop apps

@ChrisSchinnerl ChrisSchinnerl merged commit 524f08c into master Jan 17, 2025
18 checks passed
@ChrisSchinnerl ChrisSchinnerl deleted the pj/use-standard-directories branch January 17, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants