BLI: add function for changing working directory #104525

Merged
Sybren A. Stüvel merged 11 commits from SonnyCampbell_Unity/blender:change-working-directory into main 2023-02-20 15:58:49 +01:00
4 changed files with 125 additions and 6 deletions

View File

@ -142,6 +142,18 @@ double BLI_dir_free_space(const char *dir) ATTR_WARN_UNUSED_RESULT ATTR_NONNULL(
*/
char *BLI_current_working_dir(char *dir, size_t maxncpy) ATTR_WARN_UNUSED_RESULT ATTR_NONNULL();
eFileAttributes BLI_file_attributes(const char *path);
/**
* Changes the current working directory to the provided path.
*
* Usage of this function is strongly discouraged as it is not thread safe. It will likely cause
* issues if there is an operation on another thread that does not expect the current working
* directory to change. This has been added to support USDZ export, which has a problematic
* "feature" described in this issue https://projects.blender.org/blender/blender/issues/99807. It
SonnyCampbell_Unity marked this conversation as resolved

"this task" points to a D-number, so that's a patch, not a task. But, given that we've migrated, please update this to point to the right location.

"this task" points to a D-number, so that's a patch, not a task. But, given that we've migrated, please update this to point to the right location.
Review

Done

Done
* will be removed if it is possible to resolve that issue upstream in the USD library.
*
* \return true on success, false otherwise.
*/
bool BLI_change_working_dir(const char *dir);
/** \} */

View File

@ -54,11 +54,36 @@
#include "BLI_linklist.h"
#include "BLI_path_util.h"
#include "BLI_string.h"
#include "BLI_threads.h"
#include "BLI_utildefines.h"
#if !defined(__APPLE__)
SonnyCampbell_Unity marked this conversation as resolved

I think it would be nice to have a comment here that explains that the Apple implementation lives in storage_apply.mm.

I think it would be nice to have a comment here that explains that the Apple implementation lives in `storage_apply.mm`.
/* The implementation for Apple lives in storage_apple.mm.*/
bool BLI_change_working_dir(const char *dir)
{
BLI_assert(BLI_thread_is_main());
if (!BLI_is_dir(dir)) {
return false;
}
SonnyCampbell_Unity marked this conversation as resolved
Review

These (and the ones below inside the #if !defined (__APPLE__) block) should be indented (# if defined(WIN32)). clangformat should handle that for you if configured in your IDE... Otherwise please run make format before committing. ;)

These (and the ones below inside the `#if !defined (__APPLE__)` block) should be indented (`# if defined(WIN32)`). clangformat should handle that for you if [configured in your IDE](https://wiki.blender.org/wiki/Tools/ClangFormat#IDE_Integration)... Otherwise please run `make format` before committing. ;)
# if defined(WIN32)
wchar_t wdir[FILE_MAX];
if (conv_utf_8_to_16(dir, wdir, ARRAY_SIZE(wdir)) != 0) {
return false;
}
return _wchdir(wdir) == 0;
# else
int result = chdir(dir);
if (result == 0) {
BLI_setenv("PWD", dir);
}
return result == 0;
# endif
}
char *BLI_current_working_dir(char *dir, const size_t maxncpy)
{
#if defined(WIN32)
# if defined(WIN32)
wchar_t path[MAX_PATH];
if (_wgetcwd(path, MAX_PATH)) {
if (BLI_strncpy_wchar_as_utf8(dir, path, maxncpy) != maxncpy) {
@ -66,7 +91,7 @@ char *BLI_current_working_dir(char *dir, const size_t maxncpy)
}
}
return NULL;
#else
# else
const char *pwd = BLI_getenv("PWD");
if (pwd) {
size_t srclen = BLI_strnlen(pwd, maxncpy);
@ -77,8 +102,9 @@ char *BLI_current_working_dir(char *dir, const size_t maxncpy)
return NULL;
}
return getcwd(dir, maxncpy);
#endif
# endif
}
SonnyCampbell_Unity marked this conversation as resolved
Review

Although not strictly mandatory, it's good practice to add a comment reminder of the entering condition when the closing #endif is a bit far away in code: #endif /* !defined (__APPLE__) */

Although not strictly mandatory, it's good practice to add a comment reminder of the entering condition when the closing `#endif` is a bit far away in code: `#endif /* !defined (__APPLE__) */`
#endif /* !defined (__APPLE__) */
double BLI_dir_free_space(const char *dir)
{

View File

@ -13,6 +13,8 @@
#include "BLI_fileops.h"
#include "BLI_path_util.h"
#include "BLI_string.h"
/* Extended file attribute used by OneDrive to mark placeholder files. */
static const char *ONEDRIVE_RECALLONOPEN_ATTRIBUTE = "com.microsoft.OneDrive.RecallOnOpen";
@ -185,3 +187,27 @@ const char *BLI_expand_tilde(const char *path_with_tilde)
}
return path_expanded;
}
char *BLI_current_working_dir(char *dir, const size_t maxncpy) {
/* Can't just copy to the *dir pointer, as [path getCString gets grumpy.*/
static char path_expanded[PATH_MAX];
@autoreleasepool {
NSString *path = [[NSFileManager defaultManager] currentDirectoryPath];
const size_t length = maxncpy > PATH_MAX ? PATH_MAX : maxncpy;
[path getCString:path_expanded maxLength:length encoding:NSUTF8StringEncoding];
BLI_strncpy(dir, path_expanded, maxncpy);
return path_expanded;
}
}
bool BLI_change_working_dir(const char* dir) {
@autoreleasepool {
NSString* path = [[NSString alloc] initWithUTF8String: dir];
if ([[NSFileManager defaultManager] changeCurrentDirectoryPath: path] == YES) {
return false;
}
else {
return true;
}
}
}

View File

@ -1,11 +1,27 @@
/* SPDX-License-Identifier: Apache-2.0 */
#include "BLI_fileops.hh"
#include "testing/testing.h"
#include "BLI_fileops.hh"
#include "BLI_path_util.h"
#include "BLI_string.h"
#include "BLI_threads.h"
namespace blender::tests {
class ChangeWorkingDirectoryTest : public testing::Test {
public:
std::string test_temp_dir;
void TearDown() override
{
if (!test_temp_dir.empty()) {
BLI_delete(test_temp_dir.c_str(), true, false);
}
BLI_threadapi_exit();
}
};
TEST(fileops, fstream_open_string_filename)
{
const std::string test_files_dir = blender::tests::flags_test_asset_dir();
@ -37,4 +53,43 @@ TEST(fileops, fstream_open_charptr_filename)
/* Reading the file not tested here. That's deferred to `std::fstream` anyway. */
}
SonnyCampbell_Unity marked this conversation as resolved

As far as I'm concerned you can just call BLI_delete() unconditionally. It'll return a failure code, but that's ignored anyway. I'll leave it up to you to decide which one you feel is the best approach; leaving it as-is and just clicking on "resolve conversation" is fine too.

As far as I'm concerned you can just call `BLI_delete()` unconditionally. It'll return a failure code, but that's ignored anyway. I'll leave it up to you to decide which one you feel is the best approach; leaving it as-is and just clicking on "resolve conversation" is fine too.
Review

Will leave this one as is, if I remove the if (BLI_exists(test_temp_dir.c_str())) check and the directory doesn't exist, it prints 40: Unable to remove directory in the test output when it runs with the verbose flag, which might be confusing and perceived as an issue by a dev later on.

Will leave this one as is, if I remove the ` if (BLI_exists(test_temp_dir.c_str()))` check and the directory doesn't exist, it prints `40: Unable to remove directory` in the test output when it runs with the verbose flag, which might be confusing and perceived as an issue by a dev later on.
TEST_F(ChangeWorkingDirectoryTest, change_working_directory)
{
/* Must use because BLI_change_working_dir() checks that we are on the main thread. */
SonnyCampbell_Unity marked this conversation as resolved

Add some logging to this and the following ASSERT calls. When the test fails, it should be clear why it failed. You can do something like:

ASSERT_FALSE(result) << "a chdir to a non-existent directory is expected to fail";
Add some logging to this and the following ASSERT calls. When the test fails, it should be clear why it failed. You can do something like: ```cpp ASSERT_FALSE(result) << "a chdir to a non-existent directory is expected to fail"; ```
BLI_threadapi_init();
char original_wd[FILE_MAX];
if (!BLI_current_working_dir(original_wd, FILE_MAX)) {
FAIL() << "unable to get the current working directory";
}
const std::string temp_file_name(std::tmpnam(nullptr));
SonnyCampbell_Unity marked this conversation as resolved

This one ASSERT_TRUE() shouldn't indirectly test the result of BLI_dir_create_recursive(). Just use ASSERT_TRUE(BLI_dir_create_recursive(test_temp_dir.c_str());, that way this error message can remain straight-forward and only about the function under test.

This one `ASSERT_TRUE()` shouldn't indirectly test the result of `BLI_dir_create_recursive()`. Just use `ASSERT_TRUE(BLI_dir_create_recursive(test_temp_dir.c_str());`, that way this error message can remain straight-forward and only about the function under test.
test_temp_dir = temp_file_name + "овый";
SonnyCampbell_Unity marked this conversation as resolved

This should be using ASSERT_EQ.

This should be using `ASSERT_EQ`.
if (BLI_exists(test_temp_dir.c_str())) {
BLI_delete(test_temp_dir.c_str(), true, false);
}
ASSERT_FALSE(BLI_change_working_dir(test_temp_dir.c_str()))
<< "changing directory to a non-existent directory is expected to fail.";
ASSERT_TRUE(BLI_dir_create_recursive(test_temp_dir.c_str()))
<< "temporary directory should have been created successfully.";
ASSERT_TRUE(BLI_change_working_dir(test_temp_dir.c_str()))
<< "temporary directory should succeed changing directory.";
SonnyCampbell_Unity marked this conversation as resolved

This should be done in a teardown routine, so that the environment is always cleaned up when the test is done.

This should be done in a teardown routine, so that the environment is always cleaned up when the test is done.
char cwd[FILE_MAX];
if (!BLI_current_working_dir(cwd, FILE_MAX)) {
SonnyCampbell_Unity marked this conversation as resolved

Same here, BLI_threadapi_exit() should always be called, also on test failure.

Same here, `BLI_threadapi_exit()` should always be called, also on test failure.
FAIL() << "unable to get the current working directory";
}
ASSERT_EQ(BLI_path_cmp_normalized(cwd, test_temp_dir.c_str()), 0)
<< "the path of the current working directory should equal the path of the temporary "
"directory that was created.";
ASSERT_TRUE(BLI_change_working_dir(original_wd))
<< "changing directory back to the original working directory should succeed.";
}
} // namespace blender::tests