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

Make the cache directory configurable too #4103

Open
wants to merge 2 commits into
base: master
from

Conversation

@GuillaumeLestringant
Copy link

commented Jun 3, 2019

This PR adds a pair of command-line options (--usercache-path and --usercache-dir), that allow to define a custom directory for the cache or view the current cache directory.

They are exact counterparts of the --userconfig-dir and so on options and work in the exact same way.

The goal is to make the application fully portable by not letting any unnecessary hard-coded path.

@@ -734,6 +734,19 @@ void set_user_config_dir(const std::string& newconfigdir)
set_user_config_path(newconfigdir);
}

static void set_cache_path(bfs::path newcache)

This comment has been minimized.

Copy link
@Vultraz

Vultraz Jun 5, 2019

Member

How come you created this helper function that's only used once?

This comment has been minimized.

Copy link
@GuillaumeLestringant

GuillaumeLestringant Jun 5, 2019

Author

I blindly copied the way set_user_config_dir et al. work, without noticing that get_user_config_dir calls set_user_config_path, while get_cache_dir directly calls create_directory_if_missing_recursive.

There are three options.

  • Fuse set_cache_dir and set_cache_path together. But then, we would not have the same error message when the cache dir cannot be created when called from get_cache_dir.
  • Just use set_cache_path inside get_cache_dir instead of create_directory_if_missing_recursive.
  • Create a few helper functions to factorize the code of the *_cache_* and *_user_config_* functions, which are nearly identical.

Whichever you deem better.

This comment has been minimized.

Copy link
@GregoryLundberg

GregoryLundberg Jul 3, 2019

Contributor

Creating a few helper functions is good, provided they are truly common and not trivial.

Using a function only once is perfectly fine ... provided it improves readability and maintainability. The cost if a few quadrillion wasted machine cycles calling a function is far overshadowed by the cost of one programmer wasting a couple hours trying to grok what a mess of spaghetti code does.

This comment has been minimized.

Copy link
@GuillaumeLestringant

GuillaumeLestringant Jul 3, 2019

Author

The code with helper functions would be as follows.

static void create_x_dir(const bfs::path path, const char* name) {
	if(!create_directory_if_missing_recursive(path)) {
		ERR_FS << "could not open or create " << name << " directory at " << path.string() << '\n';
	} // TODO Seems to silently fail if permissions are missing to create the directory.
}

void set_user_config_dir(const std::string& newconfigdir)
{
	user_config_dir = newconfigdir;
	create_x_dir(user_config_dir, "user config");
}

void set_cache_dir(const std::string& newcachedir)
{
	cache_dir = newcachedir;
	create_x_dir(cache_dir, "cache");
}

static std::string get_x_dir(bfs::path& dir, const char* name, const char* path, const char* env)
{
	if(dir.empty()) {
#if defined(_X11) && !defined(PREFERENCES_DIR)
		char const* xdg = getenv(env);

		if(!xdg || xdg[0] == '\0') {
			char const* home_str = getenv("HOME");
			if(!home_str) {
				dir = get_user_data_path();
				if(strcmp(name, "cache") == 0)
					dir /= "cache";
				return dir.string();
			}

			dir = home_str;
			dir /= path;
		} else {
			dir = xdg;
		}

		dir /= "wesnoth";
		create_x_dir(dir, name);
#else
		dir = get_user_data_path();
		if(strcmp(name, "cache") == 0)
			dir /= "cache";
#endif
	}

	return dir.string();
}

std::string get_user_config_dir()
{
	return get_x_dir(user_config_dir, "user config", ".config", "XDG_CONFIG_HOME");
}

std::string get_cache_dir()
{
	return get_x_dir(cache_dir, "cache", ".cache", "XDG_CACHE_HOME");
}

This comment has been minimized.

Copy link
@Wedge009

Wedge009 Aug 14, 2019

Member

Is this the only thing withholding this PR from being merged? If it counts for anything, I agree with this third option: set_user_config_path and set_user_config_dir are functionally identical to set_cache_path and set_cache_dire, and get_user_config_dir and get_cache_dir are similar enough to warrant refactoring into a get_x_dir.

@GuillaumeLestringant

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

Hello @Vultraz
Would it be possible to have an answer to my proposition?
Otherwise, I'll just go with the third option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.