-
Notifications
You must be signed in to change notification settings - Fork 427
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
[Filesystem] Path Manager Improvements #4557
Conversation
Putting this in draft as we may make additional tweaks to this |
common/path_manager.cpp
Outdated
if (File::Exists(fs::path{m_server_path + "/" + dir}.string())) { | ||
return fs::relative(fs::path{m_server_path + "/" + dir}).lexically_normal().string(); | ||
} | ||
|
||
// absolute | ||
if (File::Exists(fs::path{dir}.string())) { | ||
return fs::absolute(fs::path{dir}).string(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using std::filesystem::path to join instead of string concatenation will handle both of these cases:
fs::path server_path = fs::path(m_server_path);
std::error_code ec;
if (fs::path dir_path = server_path / dir; fs::is_directory(dir_path, ec) {
return fs::proximate(dir_path).lexically_normal().string()
}
this can also be applied to the loop below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the suggestion to use native join syntax versus the string format. Done.
I do actually want to return two distinctly separate relative versus absolute forms because -
- In log messages relative reads easier eg
logs
vs/home/eqemu/server/logs
- If the folder is not in a sub-directory of the server folder we display absolute
/tmp/eq-logs/
if (!File::Exists(path.GetLogPath())) { | ||
LogInfo("Logs directory not found, creating [{}]", path.GetLogPath()); | ||
File::Makedir(path.GetLogPath()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directory creation occurs too late, because path manager would have already passed on setting the directory (due to it not existing), so path.GetLogPath()
will return empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually the case, the path resolver returns the exact value you set in your config if none of the relative or absolute directory checks succeed.
See resolver (return dir
)
auto resolve_path = [&](const std::string& dir, const std::vector<std::string>& fallback_dirs = {}) -> std::string {
// relative
if (File::Exists((fs::path{m_server_path} / dir).string())) {
return fs::relative(fs::path{m_server_path} / dir).lexically_normal().string();
}
// absolute
if (File::Exists(fs::path{dir}.string())) {
return fs::absolute(fs::path{dir}).string();
}
// fallback search options if specified
for (const auto& fallback : fallback_dirs) {
if (File::Exists((fs::path{m_server_path} / fallback).string())) {
return fs::relative(fs::path{m_server_path} / fallback).lexically_normal().string();
}
}
// if all else fails, just set it to the config value
return dir;
};
And then at this stage of the code the logging code that this is in will make sure that the path exists before it tries to write to it. In this case the path didn't exist and it creates the directory before the file is created. This is highlighted in the PR description notes.
"directories": {
"logs": "/tmp/eq-logs"
}
Zone | Info | StartFileLogs Logs directory not found, creating [/tmp/eq-logs]
Zone | Info | StartFileLogs Starting File Log [/tmp/eq-logs/zone/zone_5448.log]
Description
This surfaced from some issues Brainiac was running into when trying to set his server up a certain way and aims to resolve the following scenarios
Brainiac wants to be able to set absolute paths for most things, our current limited relative-only path loading falls short of being able to do that.
Current Loading Hierarchy
Type of change
Testing
Tested by setting
logs
to a custom absolute path directoryPaths being loaded
Creating log directory if does not exist
Checklist