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.

Sign up for GitHub

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

CVS-154703, Implement createTempPath(std::string* local_path) for windows #2879

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
+122 −35
Open

CVS-154703, Implement createTempPath(std::string* local_path) for windows #2879

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fbab2f2
CVS-154703, Implement createTempPath(std::string* local_path) for win…
18582088138 Nov 27, 2024
9b52998
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Dec 3, 2024
95aca2f
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Dec 9, 2024
730a2d1
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Jan 3, 2025
d0b39f3
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Jan 7, 2025
38fa6c4
Fix createTempPath(std::string* local_path) compile issue and impleme…
18582088138 Jan 8, 2025
b4c76bf
Fix code style
18582088138 Jan 8, 2025
b0f90d3
Fix linux build issue
18582088138 Jan 8, 2025
361af23
Fix linux build issue
18582088138 Jan 8, 2025
d867f55
Check the LocalFileSystem* unit test
18582088138 Jan 9, 2025
48e559d
SetPath tests
rasapala Jan 9, 2025
77aeb4d
SKIPPED the FileSystem.SetPath unit test on windows
18582088138 Jan 9, 2025
a27303c
Fix code style
18582088138 Jan 9, 2025
5d0c0cf
Merge branch 'cvs154703-win-createTempPath' of https://github.com/ope…
rasapala Jan 9, 2025
fbf9a93
Fix code style
18582088138 Jan 9, 2025
4645579
Add logging the ERROR message
18582088138 Jan 9, 2025
3eae2db
Fix code style
18582088138 Jan 9, 2025
5cf7da9
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Jan 10, 2025
c06c9c4
Unified the logging format
18582088138 Jan 10, 2025
e8578e6
Fix code style
18582088138 Jan 10, 2025
12e3bb0
Fix code style
18582088138 Jan 10, 2025
05a9c67
Remove reduce code
18582088138 Jan 10, 2025
32febc6
Merge branch 'main' into cvs154703-win-createTempPath
18582088138 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1874,11 +1874,11 @@ cc_test(
"test/tfs_rest_parser_row_test.cpp",
"test/threadsafequeue_test.cpp",
"test/unit_tests.cpp",
"test/localfilesystem_test.cpp",
] + select({
"//:not_disable_cloud": [
"test/gcsfilesystem_test.cpp",
"test/azurefilesystem_test.cpp",
"test/localfilesystem_test.cpp",
],
"//:disable_cloud": []
})
Expand Down
38 changes: 38 additions & 0 deletions src/filesystem.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,44 @@ class FileSystem {

*local_path = std::string(tmp_folder);

return StatusCode::OK;
}
#elif _WIN32
static StatusCode createTempPath(std::string* local_path) {
if (!local_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error path should be in { } for the local_path check

return StatusCode::FILESYSTEM_ERROR;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets add log here that target temp path is not set: SPDLOG_LOGGER_ERROR(modelmanager_logger, "Target path variable for createTempPAth not set. {}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

following the commit and the modify the code


wchar_t temp_path[MAX_PATH];
wchar_t temp_file[MAX_PATH];

DWORD path_len = GetTempPathW(MAX_PATH, temp_path);
if (path_len == 0 || path_len > MAX_PATH) {
SPDLOG_LOGGER_ERROR(modelmanager_logger, "Failed to get temp path: {}", GetLastError());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add logging error message:
DWORD error = GetLastError();
std::string message = std::system_category().message(error);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add message to log in all LastError cases.

return StatusCode::FILESYSTEM_ERROR;
}

UINT unique_num = GetTempFileNameW(temp_path, L"file", 0, temp_file);
if (unique_num == 0) {
SPDLOG_LOGGER_ERROR(modelmanager_logger, "Failed to create temp file: {}", GetLastError());
return StatusCode::FILESYSTEM_ERROR;
}

if (!DeleteFileW(temp_file)) {
SetLastError(0);
DeleteFileW(temp_file);
SPDLOG_LOGGER_ERROR(modelmanager_logger, "Failed to delete temp file: {}", GetLastError());
return StatusCode::FILESYSTEM_ERROR;
}

if (!CreateDirectoryW(temp_file, NULL)) {
SetLastError(0);
DeleteFileW(temp_file);
SPDLOG_LOGGER_ERROR(modelmanager_logger, "Failed to create temp directory: {}", GetLastError());
return StatusCode::FILESYSTEM_ERROR;
}

*local_path = fs::path(temp_file).generic_string();

return StatusCode::OK;
}
#endif
Expand Down
107 changes: 73 additions & 34 deletions src/test/localfilesystem_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,31 +27,39 @@
using namespace testing;
using ::testing::UnorderedElementsAre;

const std::string TMP_PATH = "/tmp/structure/";
const std::string TMP_FILE = "file1.txt";
#ifdef __linux__
const std::filesystem::path TMP_PATH = "/tmp/structure/";
const std::string TMP_CONTENT = "filecontent123\n";
#elif _WIN32
const std::filesystem::path TMP_PATH = std::filesystem::temp_directory_path() / "structure";
const std::string TMP_CONTENT = "filecontent123\r\n";
const std::string TMP_DIR1 = "dir1";
const std::string TMP_DIR2 = "dir2";
#endif

const std::filesystem::path TMP_FILE = "file.txt";
const std::filesystem::path TMP_FILE1 = "file1.txt";
const std::filesystem::path TMP_DIR1 = "dir1";
const std::filesystem::path TMP_DIR2 = "dir2";
const std::filesystem::path TMP_DIR5345 = "dir5345";

static void createTmpFiles() {
std::ofstream configFile(TMP_PATH + TMP_FILE);
std::ofstream configFile((TMP_PATH / TMP_FILE1).string());
configFile << TMP_CONTENT << std::endl;
configFile.close();

std::filesystem::create_directories(TMP_PATH + TMP_DIR1);
std::filesystem::create_directories(TMP_PATH + TMP_DIR2);
std::filesystem::create_directories(TMP_PATH / TMP_DIR1);
std::filesystem::create_directories(TMP_PATH / TMP_DIR2);
}

TEST(LocalFileSystem, FileExists) {
ovms::LocalFileSystem lfs;
bool exists = false;
createTmpFiles();

auto status = lfs.fileExists("/tmp/structure/file.txt", &exists);
auto status = lfs.fileExists((TMP_PATH / TMP_FILE).string(), &exists);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(exists, false);

status = lfs.fileExists("/tmp/structure/dir1", &exists);
status = lfs.fileExists((TMP_PATH / TMP_DIR1).string(), &exists);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(exists, true);
}
Expand All @@ -61,15 +69,15 @@ TEST(LocalFileSystem, IsDirectory) {
bool isDir = false;
createTmpFiles();

auto status = lfs.isDirectory("/tmp/structure/file.txt", &isDir);
auto status = lfs.isDirectory((TMP_PATH / TMP_FILE).string(), &isDir);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(isDir, false);

status = lfs.isDirectory("/tmp/structure/dir1", &isDir);
status = lfs.isDirectory((TMP_PATH / TMP_DIR1).string(), &isDir);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(isDir, true);

status = lfs.isDirectory("/tmp/structure/dir5345", &isDir);
status = lfs.isDirectory((TMP_PATH / TMP_DIR5345).string(), &isDir);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(isDir, false);
}
Expand All @@ -79,13 +87,13 @@ TEST(LocalFileSystem, GetDirectoryContents) {
ovms::files_list_t files;
createTmpFiles();

auto status = lfs.getDirectoryContents("/tmp/structure/file.txt", &files);
auto status = lfs.getDirectoryContents((TMP_PATH / TMP_FILE).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectoryContents("/tmp/structure/file1.txt", &files);
status = lfs.getDirectoryContents((TMP_PATH / TMP_FILE1).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectoryContents("/tmp/structure/", &files);
status = lfs.getDirectoryContents((TMP_PATH).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(files.size(), 3);
}
Expand All @@ -95,13 +103,13 @@ TEST(LocalFileSystem, GetDirectorySubdirs) {
ovms::files_list_t files;
createTmpFiles();

auto status = lfs.getDirectorySubdirs("/tmp/structure/file.txt", &files);
auto status = lfs.getDirectorySubdirs((TMP_PATH / TMP_FILE).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectorySubdirs("/tmp/structure/file1.txt", &files);
status = lfs.getDirectorySubdirs((TMP_PATH / TMP_FILE1).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectorySubdirs("/tmp/structure/", &files);
status = lfs.getDirectorySubdirs((TMP_PATH).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(files.size(), 2);
}
Expand All @@ -111,36 +119,38 @@ TEST(LocalFileSystem, GetDirectoryFiles) {
ovms::files_list_t files;
createTmpFiles();

auto status = lfs.getDirectoryFiles("/tmp/structure/file.txt", &files);
auto status = lfs.getDirectoryFiles((TMP_PATH / TMP_FILE).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectoryFiles("/tmp/structure/file1.txt", &files);
status = lfs.getDirectoryFiles((TMP_PATH / TMP_FILE1).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);

status = lfs.getDirectoryFiles("/tmp/structure/", &files);
status = lfs.getDirectoryFiles((TMP_PATH).string(), &files);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(files.size(), 1);
}

TEST(LocalFileSystem, DownloadFileFolder) {
ovms::LocalFileSystem lfs;
std::string location;
auto status = lfs.downloadFileFolder("/path/to/download", location);
const std::filesystem::path TMP_DOWNLOAD = "download";
auto status = lfs.downloadFileFolder((TMP_PATH / TMP_DOWNLOAD).string(), location);
// auto status = lfs.downloadFileFolder("/path/to/download", location);
EXPECT_EQ(status, ovms::StatusCode::OK);
}

TEST(LocalFileSystem, DestroyFileFolder) {
ovms::LocalFileSystem lfs;
bool exists = false;
auto status = lfs.fileExists("/tmp/structure/dir1", &exists);
auto status = lfs.fileExists((TMP_PATH / TMP_DIR1).string(), &exists);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(exists, true);
status = lfs.deleteFileFolder("/tmp/structure/dir1");
status = lfs.deleteFileFolder((TMP_PATH / TMP_DIR1).string());
EXPECT_EQ(status, ovms::StatusCode::OK);
status = lfs.fileExists("/tmp/structure/dir1", &exists);
status = lfs.fileExists((TMP_PATH / TMP_DIR1).string(), &exists);
EXPECT_EQ(status, ovms::StatusCode::OK);
EXPECT_EQ(exists, false);
status = lfs.deleteFileFolder("/tmp/structure/dir1");
status = lfs.deleteFileFolder((TMP_PATH / TMP_DIR1).string());
EXPECT_EQ(status, ovms::StatusCode::PATH_INVALID);
}

Expand All @@ -153,10 +163,13 @@ TEST(FileSystem, CreateTempFolder) {
bool status = fs::exists(local_path);
EXPECT_TRUE(status);
EXPECT_EQ(sc, ovms::StatusCode::OK);

#ifdef __linux__
fs::perms p = fs::status(local_path).permissions();
EXPECT_TRUE((p & fs::perms::group_read) == fs::perms::none);
EXPECT_TRUE((p & fs::perms::others_read) == fs::perms::none);
EXPECT_TRUE((p & fs::perms::owner_read) != fs::perms::none);
#endif
}

TEST(FileSystem, CheckIfPathIsEscaped) {
Expand Down Expand Up @@ -186,33 +199,37 @@ TEST(FileSystem, SetRootDirectoryPath) {
std::string rootPath = "";
std::string givenPath = "/givenpath";

auto normalize_path = [](const std::string& path) -> std::string {
return std::filesystem::weakly_canonical(std::filesystem::path(path)).string();
};

ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, "/");
ASSERT_EQ(normalize_path(rootPath), normalize_path("/"));

givenPath = "/givenpath/longer";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, "/givenpath/");
ASSERT_EQ(normalize_path(rootPath), normalize_path("/givenpath/"));

givenPath = "/givenpath/longer/somefile.txt";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, "/givenpath/longer/");
ASSERT_EQ(normalize_path(rootPath), normalize_path("/givenpath/longer/"));

givenPath = "givenpath";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
std::string currentWorkingDir = std::filesystem::current_path();
ASSERT_EQ(rootPath, ovms::FileSystem::joinPath({currentWorkingDir, ""}));
std::string currentWorkingDir = std::filesystem::current_path().string();
ASSERT_EQ(normalize_path(rootPath), normalize_path(ovms::FileSystem::joinPath({currentWorkingDir, ""})));

givenPath = "/givenpath/";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, givenPath);
ASSERT_EQ(normalize_path(rootPath), normalize_path(givenPath));

givenPath = "1";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, ovms::FileSystem::joinPath({currentWorkingDir, ""}));
ASSERT_EQ(normalize_path(rootPath), normalize_path(ovms::FileSystem::joinPath({currentWorkingDir, ""})));

givenPath = "";
ovms::FileSystem::setRootDirectoryPath(rootPath, givenPath);
ASSERT_EQ(rootPath, ovms::FileSystem::joinPath({currentWorkingDir, ""}));
ASSERT_EQ(normalize_path(rootPath), normalize_path(ovms::FileSystem::joinPath({currentWorkingDir, ""})));
}

TEST(FileSystem, SetPath) {
Expand All @@ -225,30 +242,52 @@ TEST(FileSystem, SetPath) {
} catch (std::logic_error& e) {
}

#ifdef __linux__
rootPath = "/rootPath";
#elif _WIN32
rootPath = "C:\\rootPath";
#endif
testPath = "";
givenPath = "";

ovms::FileSystem::setPath(testPath, givenPath, rootPath);
ASSERT_EQ(testPath, rootPath);

testPath = "";
#ifdef __linux__
givenPath = "/givenPath";
#elif _WIN32
givenPath = "C:\\givenPath";
#endif

ovms::FileSystem::setPath(testPath, givenPath, rootPath);
#ifdef __linux__
ASSERT_EQ(testPath, "/givenPath");
#elif _WIN32
ASSERT_EQ(testPath, "C:\\givenPath");
#endif


testPath = "";
givenPath = "givenPath";

ovms::FileSystem::setPath(testPath, givenPath, rootPath);
#ifdef __linux__
ASSERT_EQ(testPath, "/rootPathgivenPath");
#elif _WIN32
ASSERT_EQ(testPath, "C:\\rootPathgivenPath");
#endif

testPath = "";
givenPath = "long/givenPath";

ovms::FileSystem::setPath(testPath, givenPath, rootPath);

#ifdef __linux__
ASSERT_EQ(testPath, "/rootPathlong/givenPath");
#elif _WIN32
ASSERT_EQ(testPath, "C:\\rootPathlong/givenPath");
#endif

testPath = "";
givenPath = "s3://long/givenPath";
Expand Down