BLI: add function for changing working directory #104525
|
@ -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
|
||||
* 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);
|
||||
|
||||
/** \} */
|
||||
|
||||
|
|
|
@ -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
Sybren A. Stüvel
commented
I think it would be nice to have a comment here that explains that the Apple implementation lives in 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
Bastien Montagne
commented
These (and the ones below inside the 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
Bastien Montagne
commented
Although not strictly mandatory, it's good practice to add a comment reminder of the entering condition when the closing 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)
|
||||
{
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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
Sybren A. Stüvel
commented
As far as I'm concerned you can just call 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.
Sonny Campbell
commented
Will leave this one as is, if I remove the 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
Sybren A. Stüvel
commented
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:
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
Sybren A. Stüvel
commented
This one 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
Sybren A. Stüvel
commented
This should be using 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
Sybren A. Stüvel
commented
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
Sybren A. Stüvel
commented
Same here, 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
|
||||
|
|
"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.
Done