blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 765 Projects 19 Wiki Activity

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
Conversation 3 Commits 11 Files Changed 4 +19 -11
2 changed files with 19 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 74de514dcc - Show all commits

4
source/blender/blenlib/BLI_fileops.h
View File

@ -148,8 +148,8 @@ eFileAttributes BLI_file_attributes(const char *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 task https://developer.blender.org/D15623. It will be removed if it
* is possible to resolve that issue upstream in the USD library.
* "feature" described in this issue https://projects.blender.org/blender/blender/issues/99807. It
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:08:58 +01:00
Review
Copy Link

"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.
Sonny Campbell commented 2023-02-13 15:25:50 +01:00
Review
Copy Link

Done

Done
* will be removed if it is possible to resolve that issue upstream in the USD library.
*
* \return true on success, false otherwise.
*/

26
source/blender/blenlib/tests/BLI_fileops_test.cc
View File

@ -1,5 +1,4 @@
/* SPDX-License-Identifier: Apache-2.0 */
#include "testing/testing.h"
#include "BLI_fileops.hh"
@ -42,33 +41,42 @@ TEST(fileops, fstream_open_charptr_filename)
TEST(fileops, change_working_directory)
{
/* Must use because BLI_change_working_dir() checks that we are on the main thread. */
BLI_threadapi_init();
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:15:02 +01:00
Review
Copy Link

Add a comment to explain why this test needs the BLI_threadapi_...() calls.

Add a comment to explain why this test needs the `BLI_threadapi_...()` calls.
Sonny Campbell commented 2023-02-13 15:25:35 +01:00
Review
Copy Link

Done

Done
char original_wd[FILE_MAX];
BLI_current_working_dir(original_wd, FILE_MAX);
if (!BLI_current_working_dir(original_wd, FILE_MAX)) {
FAIL() << "unable to get the current working directory";
}
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:12:51 +01:00
Review
Copy Link

Tests shouldn't write to the test assets directory. Use a proper temp dir function for this.

Tests shouldn't write to the test assets directory. Use a proper temp dir function for this.
Sonny Campbell commented 2023-02-10 16:38:03 +01:00
Review
Copy Link

I originally created a temporary directory in place, and Bastien suggested using flags_test_release_dir, or flags_test_asset_dir, or creating a new flag to pass into the tests as something like flags_test_temp_dir.

What do you want me to use?

I originally created a temporary directory in place, and Bastien suggested using `flags_test_release_dir`, or `flags_test_asset_dir`, or creating a new flag to pass into the tests as something like `flags_test_temp_dir`. What do you want me to use?
Sybren A. Stüvel commented 2023-02-13 11:50:10 +01:00
Review
Copy Link

I checked with him, and his intention was to use a mechanism like used with the flags_test_asset_dir; that is, to add another flag to point to a specific directory, but not to literally reuse that particular flag.

I'd suggest calling BKE_tempdir_init(nullptr); at the start of the test, and then use a combination of BKE_tempdir_base(); and std::tmpnam() to generate a name that's guaranteed to be unique.

That'll avoid writing to the SVN directory, and also avoids the need to have a CLI flag for this.

I checked with him, and his intention was to use a mechanism like used with the `flags_test_asset_dir`; that is, to add another flag to point to a specific directory, but not to literally reuse that particular flag. I'd suggest calling `BKE_tempdir_init(nullptr);` at the start of the test, and then use a combination of `BKE_tempdir_base();` and `std::tmpnam()` to generate a name that's guaranteed to be unique. That'll avoid writing to the SVN directory, and also avoids the need to have a CLI flag for this.
Sonny Campbell commented 2023-02-13 12:49:58 +01:00
Review
Copy Link

Thanks, that's perfect. Will get it updated.

Thanks, that's perfect. Will get it updated.
const std::string test_temp_dir = blender::tests::flags_test_asset_dir() + "/" +
"fileops_test_temp_новый";
const std::string temp_file_name(std::tmpnam(nullptr));
const std::string test_temp_dir = temp_file_name + "_новый";
if (BLI_exists(test_temp_dir.c_str())) {
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:51:22 +01:00
Review
Copy Link

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.
Sonny Campbell commented 2023-02-17 11:11:32 +01:00
Review
Copy Link

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.
👍 1
BLI_delete(test_temp_dir.c_str(), true, false);
}
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:14:08 +01:00
Review
Copy Link

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"; ```
bool result = BLI_change_working_dir(test_temp_dir.c_str());
ASSERT_FALSE(result);
ASSERT_FALSE(result) << "changing directory to a non-existent directory is expected to fail.";
BLI_dir_create_recursive(test_temp_dir.c_str());
result = BLI_change_working_dir(test_temp_dir.c_str());
ASSERT_TRUE(result);
ASSERT_TRUE(result)
<< "temporary directory should have been created and should succeed changing directory.";
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:52:53 +01:00
Review
Copy Link

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.
char cwd[FILE_MAX];
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:14:30 +01:00
Review
Copy Link

This should be using ASSERT_EQ.

This should be using `ASSERT_EQ`.
BLI_current_working_dir(cwd, FILE_MAX);
if (!BLI_current_working_dir(cwd, FILE_MAX)) {
FAIL() << "unable to get the current working directory";
}
ASSERT_TRUE(BLI_path_cmp_normalized(cwd, test_temp_dir.c_str()) == 0);
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.";
result = BLI_change_working_dir(original_wd);
ASSERT_TRUE(result);
ASSERT_TRUE(result)
<< "changing directory back to the original working directory should succeed.";
BLI_delete(test_temp_dir.c_str(), true, false);
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:54:09 +01:00
Review
Copy Link

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.