refine handling of XDG directory resolution

[kevinushey]

Intent

Addresses https://github.com/rstudio/rstudio-pro/issues/5658.

Approach

• Re-organize code for greater clarity.
• Use the XDG directory provided by the RStudio-specific variable unconditionally.
• Only use the XDG home directory if the RStudio data subdirectory exists.

Automated Tests

TODO

QA Notes

Test via notes in https://github.com/rstudio/rstudio-pro/issues/5658.

Documentation

N/A

Checklist

• If this PR adds a new feature, or fixes a bug in a previously released version, it includes an entry in NEWS.md
• If this PR adds or changes UI, the updated UI meets accessibility standards
• A reviewer is assigned to this PR (if unsure who to assign, check Area Owners list)
• This PR passes all local unit tests

[kevinushey]

My main question is around what we should do in this scenario. Using

rstudio/src/cpp/core/system/Xdg.cpp

Lines 206 to 219 in 7ecae3c

	FilePath userConfigDir(
	const boost::optional<std::string>& user,
	const boost::optional<FilePath>& homeDir)
	{
	return resolveXdgDir("RSTUDIO_CONFIG_HOME",
	"XDG_CONFIG_HOME",
	#ifdef _WIN32
	FOLDERID_RoamingAppData,
	#endif
	"~/.config",
	user,
	homeDir
	);
	}

as an example, suppose that:

• RSTUDIO_CONFIG_HOME is not set,
• XDG_CONFIG_HOME is set, but ${XDG_CONFIG_HOME}/rstudio does not exist,
• ~/.config/rstudio also does not exist.

Which path should we use in this scenario -- ${XDG_CONFIG_HOME}/rstudio, or ~/.config/rstudio? I think we want to use ${XDG_CONFIG_HOME}/rstudio in this scenario, but it's worth confirming.

[gtritchie]

LGTM! We chatted elsewhere about theoretical performance concerns of this API now doing filesystem operations, and also that we can wait and merge this into the next release (not Chocolate Cosmos).

[gtritchie]

This if isn't strictly necessary; CoTaskMemFree can be safely called on a nullptr.

[kevinushey]

Moving to draft to avoid unintentional merge; we might prefer taking this PR in the next release rather than Chocolate Cosmos?

[kevinushey]

One other idea that arose while chatting with @gtritchie: should we be caching the computed values for XDG folder locations? It seems unlikely that we'd want to allow the user configuration directory to change during the lifetime of a process / session, but theoretically that could happen if the user were to set (for example) RSTUDIO_CACHE_HOME in a running R session.

[gtritchie]

This existing code already has logic for choosing a folder based on presence of the rstudio folder.

rstudio/src/cpp/core/system/Xdg.cpp

Lines 305 to 337 in e33a035

	FilePath systemConfigDir()
	{
	#ifndef _WIN32
	if (getenv("RSTUDIO_CONFIG_DIR").empty())
	{
	// On POSIX operating systems, it's possible to specify multiple config
	// directories. We have to select one, so read the list and take the first
	// one that contains an "rstudio" folder.
	std::string env = getenv("XDG_CONFIG_DIRS");
	if (env.find_first_of(":") != std::string::npos)
	{
	std::vector<std::string> dirs = algorithm::split(env, ":");
	for (const std::string& dir: dirs)
	{
	FilePath resolved = FilePath(dir).completePath("rstudio");
	if (resolved.exists())
	{
	return resolved;
	}
	}
	}
	}
	#endif
	return resolveXdgDir("RSTUDIO_CONFIG_DIR",
	"XDG_CONFIG_DIRS",
	#ifdef _WIN32
	FOLDERID_ProgramData,
	#endif
	"/etc",
	boost::none, // no specific user
	boost::none // no home folder resolution
	);
	}

[jeffvroom]

LGTM! This is a great cleanup. So as I understand it, we still are choosing the first config directory that's found. If a specific config file doesn't exist in that dir, the latter config dirs won't be checked. That's how it's worked in the past but if we change that, it could theoretically break things in weird ways for individual customers because of the chaos that exists in the world :)

I do think caching would be fine for this api. It's an easy win to shave a fraction of a second off startup time.

If none of the dirs exist, choosing the "first one specified" makes sense to me. Maybe you want to create it in that scenario if you are using this from an admin UI tool, but it also seems like that tool should have already done that before this api was called.

[gtritchie]

LGTM! Would adding some unit tests make sense for this?

[kevinushey]

LGTM! Would adding some unit tests make sense for this?

Good idea; let me cook something up.

[kevinushey]

I think this is the behavior we want, but it would be worth confirming.

[jeffvroom]

I think that's right for this case because the main XDG directory does exist, but the rstudio directory inside of it does not exist. So we ignore that XDG entry for rstudio config files. This is how it used to work so I think is right here.

[kevinushey]

Same here?

[kevinushey]

I tried to remove a bit of duplicated code, and also added a number of test cases. (I also enabled stderr logging for tests while I was in that code).

Can you look through the test cases and see if the asserted behavior is what we expect? In particular, I want to make sure I have the semantics around resolving a file within an XDG directory location correct. For example, suppose we're resolving logging.conf:

• If RSTUDIO_CONFIG_DIR is set, that overrides all, and so we use ${RSTUDIO_CONFIG_DIR}/logging.conf, even if that path doesn't yet exist.
• Otherwise, we iterate through the XDG_CONFIG_DIRS. If we find a path of the form ${XDG_CONFIG_DIR}/rstudio/logging.conf, then we use it.
• If that fails, then we use the fallback XDG location, even if one of the declared XDG_CONFIG_DIRS exists. I believe this would preserve existing behavior. That is, we would use /etc/rstudio.logging.conf instead of /etc/xdg/rstudio/logging.conf, even if the /etc/xdg/rstudio were to exist. I'm not sure if this is the right approach, though.

As a side note, https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html has:

If $XDG_CONFIG_DIRS is either not set or empty, a value equal to /etc/xdg should be used.

which implies we're actually deviating from the standard a bit in using /etc here, but I don't think we want to touch that.

[gtritchie]

Can you look through the test cases and see if the asserted behavior is what we expect?

Seems reasonable to me; @jeffvroom might want to take another peek to confirm.

[gtritchie]

LGTM

[jeffvroom]

LGTM!