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

Can't create directory and file like "D:/Logs/log.txt" #3079

Closed
fanghtao opened this issue Apr 28, 2024 · 14 comments
Closed

Can't create directory and file like "D:/Logs/log.txt" #3079

fanghtao opened this issue Apr 28, 2024 · 14 comments

Comments

@fanghtao
Copy link

platform: windows
file: os-inl.h
line: 538
if (!subdir.empty() && !path_exists(subdir) && !mkdir_(subdir)) {
return false; // return error if failed creating dir
}
this code will return false when create subdir = "D:/"
std::shared_ptrspdlog::logger logger = spdlog::daily_logger_mt("log", "/Logs/log.txt") ---- create directory succeed
std::shared_ptrspdlog::logger logger = spdlog::daily_logger_mt("log", "D:/Logs/log.txt") ---- can't create directory

@tt4g
Copy link
Contributor

tt4g commented Apr 28, 2024

It may have something to do with the fact that the Windows path separator is \ and some Windows APIs do not support /.

-"D:/Logs/log.txt"
+"D:\\Logs\\log.txt"

@fanghtao
Copy link
Author

fanghtao commented Apr 28, 2024

Thank you for your response but it's not a problem of windows path separator.

std::shared_ptrspdlog::logger logger = spdlog::daily_logger_mt("Log", "D:\\Logs\\log.txt");
file: os-inl.h
line: 538
if (!subdir.empty() && !path_exists(subdir) && !mkdir_(subdir)) {
return false; // return error if failed creating dir
}
this code will return false when create subdir = "D:" (not "D:/" or "D:\\")

@gabime
Copy link
Owner

gabime commented Apr 28, 2024

Could be a permissions problem.

@fanghtao
Copy link
Author

I'm sure it's not a permissions problem becasuse spdlog::daily_logger_mt("Log", "\\Logs\\log.txt") is ok and in fact they are the same path on my computer.

file: os-inl.h
line: 536

  • auto subdir = path.substr(0, token_pos); this code will create "D:" but it's not a directory
    +auto subdir = path.substr(0, token_pos + 1);

@tt4g
Copy link
Contributor

tt4g commented Apr 28, 2024

auto subdir = path.substr(0, token_pos); this code will create "D:" but it's not a directory
+auto subdir = path.substr(0, token_pos + 1);

This fix causes a problem on UNIX-like systems.

The problem should be considered that _stat() called in path_exists() fails when only the drive letter character D: is passed.

if (!subdir.empty() && !path_exists(subdir) && !mkdir_(subdir)) {
return false; // return error if failed creating dir
}

SPDLOG_INLINE bool path_exists(const filename_t &filename) SPDLOG_NOEXCEPT {
#ifdef _WIN32
struct _stat buffer;
#ifdef SPDLOG_WCHAR_FILENAMES
return (::_wstat(filename.c_str(), &buffer) == 0);
#else
return (::_stat(filename.c_str(), &buffer) == 0);
#endif
#else // common linux/unix all have the stat system call
struct stat buffer;
return (::stat(filename.c_str(), &buffer) == 0);
#endif
}

Can you try to complete \ when filename is a drive letter character and see if it works?
If it works, could you send the PR.

@fanghtao
Copy link
Author

1
2
3
4
5
6

@tt4g
Copy link
Contributor

tt4g commented Apr 29, 2024

Since the Windows drive letter specification is one of A-Z, can't we fix this with a hack that if _stat() fails and the next condition is met, then _stat() again with the \ character added?

  • 2 characters
  • First character is a-zA-Z.
  • Last character is :

@fanghtao
Copy link
Author

I don't know what effect this will have on UNIX-like systems, but _stat("D:/") and _mkdir("D:/") or _stat("D:/XX/") and _mkdir("D:/XX/") works on Windows.
maybe we can do like this:

#ifdef _WIN32
auto subdir = path.substr(0, token_pos + 1);
#else
auto subdir = path.substr(0, token_pos);
#endif // _WIN32

@tt4g
Copy link
Contributor

tt4g commented Apr 29, 2024

The behavior when a path is a symbolic link differs depending on whether or not a trailing slash is used.
This may affect Windows as well, since recent Windows OS supports symbolic links.

@tt4g
Copy link
Contributor

tt4g commented Apr 29, 2024

You must also consider the case where a log file is to be created directly under the drive (e.g. D:\log.txt).
In that case, only "D:" is passed to create_dir(), so your fix cannot cover this pattern.

// Return directory name from given path or empty string
// "abc/file" => "abc"
// "abc/" => "abc"
// "abc" => ""
// "abc///" => "abc//"
SPDLOG_INLINE filename_t dir_name(const filename_t &path) {
auto pos = path.find_last_of(folder_seps_filename);
return pos != filename_t::npos ? path.substr(0, pos) : filename_t{};
}

@fanghtao
Copy link
Author

Thank you for your patient reply and answer. But it's a real problem when I use spdlog to create log files and I need to manually create the parent directory.

@tt4g
Copy link
Contributor

tt4g commented Apr 29, 2024

Please try adding to the workaround I came up with: #3079 (comment)

@gabime
Copy link
Owner

gabime commented Apr 29, 2024

@fanghtao So what was the fix ??

@gabime gabime reopened this Apr 29, 2024
@gabime gabime closed this as completed in 94a8e87 Apr 29, 2024
@fanghtao
Copy link
Author

@gabime Thank you for your fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants