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
Member

Original Review Discussion Here: https://archive.blender.org/developer/differential/0016/0016922/index.html

This patch adds functionality to change the current working directory.

This change is required for https://developer.blender.org/D15623. It was initially implemented as part of that patch, but it was requested that the blenlib changes be committed in a separate patch.

The reason it is required is that when exporting to usdz, we are required to do a weird change of directory because of a quirk with the USD conversion to usdz. If an absolute filepath is passed into the UsdUtilsCreateNewUsdzPackage function, the usd files inside the usdz archive will have the same directory structure, i.e. if I try to create a file at C:\code\BlenderProjects\file.usdz, when I create the usdz file inside it will have the structure \code\BlenderProjects\file.usdc.

This is counteracted by setting the current working directory to the temporary session directory where I create both the usdc and usdz files, and just passing the file name to UsdUtilsCreateNewUsdzPackage without any filepath. Once the usdz file is created I copy it out to the intended directory.

Set dir parameter to const for BLI_change_working_dir.
Added non-ASCII characters to the test directory.

Mac and Linux fixes. Linux now sets PWD when using BLI_change_working_dir. Mac uses NSFileManager through some new Mac-only wrapper functions.

Changed temporary directory to use a directory in the flags_test_asset_dir.
Ensure that operation to change working directory is from the main thread.

Ref #99807

Original Review Discussion Here: https://archive.blender.org/developer/differential/0016/0016922/index.html This patch adds functionality to change the current working directory. This change is required for https://developer.blender.org/D15623. It was initially implemented as part of that patch, but it was requested that the blenlib changes be committed in a separate patch. The reason it is required is that when exporting to usdz, we are required to do a weird change of directory because of a quirk with the USD conversion to usdz. If an absolute filepath is passed into the UsdUtilsCreateNewUsdzPackage function, the usd files inside the usdz archive will have the same directory structure, i.e. if I try to create a file at C:\code\BlenderProjects\file.usdz, when I create the usdz file inside it will have the structure \code\BlenderProjects\file.usdc. This is counteracted by setting the current working directory to the temporary session directory where I create both the usdc and usdz files, and just passing the file name to UsdUtilsCreateNewUsdzPackage without any filepath. Once the usdz file is created I copy it out to the intended directory. Set dir parameter to const for BLI_change_working_dir. Added non-ASCII characters to the test directory. Mac and Linux fixes. Linux now sets PWD when using BLI_change_working_dir. Mac uses NSFileManager through some new Mac-only wrapper functions. Changed temporary directory to use a directory in the flags_test_asset_dir. Ensure that operation to change working directory is from the main thread. Ref #99807
Sonny Campbell added 1 commit 2023-02-09 16:58:10 +01:00
96aa7ab367 T99807: Separate logic for changing working directory.
Set dir parameter to const for BLI_change_working_dir.
Added non-ASCII characters to the test directory.

Mac and Linux fixes. Linux now sets PWD when using BLI_change_working_dir. Mac uses NSFileManager through some new Mac-only wrapper functions.

Changed temporary directory to use a directory in the flags_test_asset_dir.
Ensure that operation to change working directory is from the main thread.
Sonny Campbell requested review from Brecht Van Lommel 2023-02-09 16:59:54 +01:00
Sonny Campbell requested review from Sybren A. Stüvel 2023-02-09 16:59:54 +01:00
Sonny Campbell requested review from Bastien Montagne 2023-02-09 16:59:54 +01:00
Brecht Van Lommel approved these changes 2023-02-09 17:22:50 +01:00
Brecht Van Lommel changed title from T99807: Separate logic for changing working directory. to BLI: separate logic for changing working directory. 2023-02-09 19:20:16 +01:00
Brecht Van Lommel changed title from BLI: separate logic for changing working directory. to BLI: separate logic for changing working directory 2023-02-09 19:20:39 +01:00
Bastien Montagne added this to the USD project 2023-02-10 11:09:45 +01:00
Bastien Montagne modified the project from USD to Core Libraries 2023-02-10 11:11:34 +01:00
Bastien Montagne added the
Interest
USD
label 2023-02-10 11:11:52 +01:00
Sybren A. Stüvel requested changes 2023-02-10 15:33:02 +01:00
Sybren A. Stüvel left a comment
Member

I'm getting compiler warnings:

blender/source/blender/blenlib/tests/BLI_fileops_test.cc: In member function ‘virtual void blender::tests::fileops_change_working_directory_Test::TestBody()’:
blender/source/blender/blenlib/tests/BLI_fileops_test.cc:48:26: warning: ignoring return value of ‘char* BLI_current_working_dir(char*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   48 |   BLI_current_working_dir(original_wd, FILE_MAX);
      |   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
blender/source/blender/blenlib/tests/BLI_fileops_test.cc:66:26: warning: ignoring return value of ‘char* BLI_current_working_dir(char*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   66 |   BLI_current_working_dir(cwd, FILE_MAX);
      |   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~

The code does build, and the tests succeed, so that's good!

I'm getting compiler warnings: ``` blender/source/blender/blenlib/tests/BLI_fileops_test.cc: In member function ‘virtual void blender::tests::fileops_change_working_directory_Test::TestBody()’: blender/source/blender/blenlib/tests/BLI_fileops_test.cc:48:26: warning: ignoring return value of ‘char* BLI_current_working_dir(char*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 48 | BLI_current_working_dir(original_wd, FILE_MAX); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~ blender/source/blender/blenlib/tests/BLI_fileops_test.cc:66:26: warning: ignoring return value of ‘char* BLI_current_working_dir(char*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 66 | BLI_current_working_dir(cwd, FILE_MAX); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ ``` The code does build, and the tests succeed, so that's good!
@ -145,0 +148,4 @@
* 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

"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.
Author
Member

Done

Done
SonnyCampbell_Unity marked this conversation as resolved
@ -313,6 +325,8 @@ void BLI_file_free_lines(struct LinkNode *lines);
* Giving a path without leading `~` is not an error.
*/
const char *BLI_expand_tilde(const char *path_with_tilde);
const char *BLI_apple_getcwd(const size_t max_length);

Why are there separate Apple-specific functions, when there's also BLI_current_working_dir() and BLI_change_working_dir()? Can't those generic function call whatever lower-level platform-specific implementation is required, instead of exposing this in the public API of BLI?

Why are there separate Apple-specific functions, when there's also `BLI_current_working_dir()` and `BLI_change_working_dir()`? Can't those generic function call whatever lower-level platform-specific implementation is required, instead of exposing this in the public API of BLI?

Hi @dr.sybren -- I did the Apple and Linux side of this patch.

BLI_apple_getcwd is in a .mm file, which is required in order to access the necessary Objective C API calls; the calls cannot be made in BLI_fileops directly.

If your objection is to having that particular function in the public header, would moving the declaration into BLI_fileops inside an APPLE #ifdef guard be an acceptable solve?

Hi @dr.sybren -- I did the Apple and Linux side of this patch. `BLI_apple_getcwd` is in a .mm file, which is required in order to access the necessary Objective C API calls; the calls cannot be made in BLI_fileops directly. If your objection is to having that particular function in the public header, would moving the declaration into BLI_fileops inside an __APPLE__ #ifdef guard be an acceptable solve?

If your objection is to having that particular function in the public header

My objection was basically two-fold:

  • A "private" implementation detail being presented as public API.
  • No documentation as to what this function does, why it's Apple-specific, and when to use it instead of other alternatives.

I think the best approach would be to give the .mm file its own private header, and include that in the fileops.c so that the Apple-specific code can be called as an implementation detail of BLI_change_working_dir().

> If your objection is to having that particular function in the public header My objection was basically two-fold: - A "private" implementation detail being presented as public API. - No documentation as to what this function does, why it's Apple-specific, and when to use it instead of other alternatives. I think the best approach would be to give the `.mm` file its own private header, and include that in the `fileops.c` so that the Apple-specific code can be called as an implementation detail of `BLI_change_working_dir()`.

Note that other functions, like BLI_expand_tilde, are fully defined in the .mm file for OSX, and not defined at all in the C file for this platform.

Since these new functions also share very little code in common for all platforms, would do the same here (i.e. directly implement BLI_current_working_dir and BLI_change_working_dir in the .mm file for OSX).

Note that other functions, like `BLI_expand_tilde`, are fully defined in the `.mm` file for OSX, and not defined at all in the C file for this platform. Since these new functions also share very little code in common for all platforms, would do the same here (i.e. directly implement `BLI_current_working_dir` and `BLI_change_working_dir` in the `.mm` file for OSX).

Are you saying you'd like us to put an #if !defined(__APPLE__) around BLI_current_working_dir in storage.c?

The function in storage.c has code for windows and code for linux, which is not possible for Mac.

Are you saying you'd like us to put an `#if !defined(__APPLE__)` around `BLI_current_working_dir` in storage.c? The function in storage.c has code for windows and code for linux, which is not possible for Mac.

Yes, exactly as e.g. BLI_file_attributes is handled.

Yes, exactly as e.g. `BLI_file_attributes` is handled.
Author
Member

@mont29 @dr.sybren I've merged the changes from Charles that were requested here

@mont29 @dr.sybren I've merged the changes from Charles that were requested here
SonnyCampbell_Unity marked this conversation as resolved
@ -39,1 +42,4 @@
TEST(fileops, change_working_directory)
{
BLI_threadapi_init();

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.
Author
Member

Done

Done
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +47,4 @@
char original_wd[FILE_MAX];
BLI_current_working_dir(original_wd, FILE_MAX);
const std::string test_temp_dir = blender::tests::flags_test_asset_dir() + "/" +

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.
Author
Member

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?

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.
Author
Member

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

Thanks, that's perfect. Will get it updated.
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +55,4 @@
}
bool result = BLI_change_working_dir(test_temp_dir.c_str());
ASSERT_FALSE(result);

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"; ```
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +65,4 @@
char cwd[FILE_MAX];
BLI_current_working_dir(cwd, FILE_MAX);
ASSERT_TRUE(BLI_path_cmp_normalized(cwd, test_temp_dir.c_str()) == 0);

This should be using ASSERT_EQ.

This should be using `ASSERT_EQ`.
SonnyCampbell_Unity marked this conversation as resolved
Sonny Campbell added 1 commit 2023-02-13 15:25:18 +01:00
Sonny Campbell added 1 commit 2023-02-15 10:52:29 +01:00
Sonny Campbell requested review from Sybren A. Stüvel 2023-02-15 10:59:51 +01:00

@blender-bot build

@blender-bot build
Bastien Montagne approved these changes 2023-02-15 11:14:30 +01:00
Bastien Montagne left a comment
Owner

Besides the two style-related comments below, this LGTM now. No need for another review from me once these are addressed.

Besides the two style-related comments below, this LGTM now. No need for another review from me once these are addressed.
@ -59,0 +65,4 @@
if (!BLI_is_dir(dir)) {
return false;
}
#if defined(WIN32)

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. ;)
SonnyCampbell_Unity marked this conversation as resolved
@ -79,6 +103,7 @@ char *BLI_current_working_dir(char *dir, const size_t maxncpy)
return getcwd(dir, maxncpy);
#endif
}
#endif

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__) */`
SonnyCampbell_Unity marked this conversation as resolved

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed change-working-directory from a7d9b4329b to 7b348a8475 2023-02-15 12:11:19 +01:00 Compare

@blender-bot build

@blender-bot build
Sonny Campbell added 5 commits 2023-02-15 13:38:33 +01:00
96aa7ab367 T99807: Separate logic for changing working directory.
Set dir parameter to const for BLI_change_working_dir.
Added non-ASCII characters to the test directory.

Mac and Linux fixes. Linux now sets PWD when using BLI_change_working_dir. Mac uses NSFileManager through some new Mac-only wrapper functions.

Changed temporary directory to use a directory in the flags_test_asset_dir.
Ensure that operation to change working directory is from the main thread.
Sybren A. Stüvel requested changes 2023-02-16 12:54:47 +01:00
@ -57,2 +57,4 @@
#include "BLI_threads.h"
#include "BLI_utildefines.h"
#if !defined(__APPLE__)

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`.
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +52,4 @@
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())) {

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.
Author
Member

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.
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +63,4 @@
result = BLI_change_working_dir(test_temp_dir.c_str());
ASSERT_TRUE(result)
<< "temporary directory should have been created and should succeed changing directory.";

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.
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +78,4 @@
ASSERT_TRUE(result)
<< "changing directory back to the original working directory should succeed.";
BLI_delete(test_temp_dir.c_str(), true, false);

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.
SonnyCampbell_Unity marked this conversation as resolved
@ -40,0 +80,4 @@
BLI_delete(test_temp_dir.c_str(), true, false);
BLI_threadapi_exit();

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.
SonnyCampbell_Unity marked this conversation as resolved
Sonny Campbell added 1 commit 2023-02-17 11:34:51 +01:00
Sonny Campbell added 1 commit 2023-02-17 11:39:28 +01:00
Sonny Campbell requested review from Sybren A. Stüvel 2023-02-17 11:40:22 +01:00
Sybren A. Stüvel approved these changes 2023-02-20 11:43:44 +01:00
Sybren A. Stüvel changed title from BLI: separate logic for changing working directory to BLI: add function for changing working directory 2023-02-20 15:15:12 +01:00
Sybren A. Stüvel added 1 commit 2023-02-20 15:15:59 +01:00
Sybren A. Stüvel merged commit ceba1854f9 into main 2023-02-20 15:58:49 +01:00
Sybren A. Stüvel deleted branch change-working-directory 2023-02-20 15:58:49 +01:00
Bastien Montagne removed this from the Core Libraries project 2023-03-08 09:39:44 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104525
No description provided.