BLI: add function for changing working directory #104525
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104525
Loading…
Reference in New Issue
No description provided.
Delete Branch "SonnyCampbell_Unity/blender:change-working-directory"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
T99807: Separate logic for changing working directory.to BLI: separate logic for changing working directory.BLI: separate logic for changing working directory.to BLI: separate logic for changing working directoryI'm getting compiler warnings:
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.
Done
@ -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()
andBLI_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?
My objection was basically two-fold:
I think the best approach would be to give the
.mm
file its own private header, and include that in thefileops.c
so that the Apple-specific code can be called as an implementation detail ofBLI_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
andBLI_change_working_dir
in the.mm
file for OSX).Are you saying you'd like us to put an
#if !defined(__APPLE__)
aroundBLI_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.@mont29 @dr.sybren I've merged the changes from Charles that were requested here
@ -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.Done
@ -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.
I originally created a temporary directory in place, and Bastien suggested using
flags_test_release_dir
, orflags_test_asset_dir
, or creating a new flag to pass into the tests as something likeflags_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 ofBKE_tempdir_base();
andstd::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.
Thanks, that's perfect. Will get it updated.
@ -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:
@ -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
.@blender-bot build
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 runmake format
before committing. ;)@ -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__) */
@blender-bot build
a7d9b4329b
to7b348a8475
@blender-bot build
@ -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
.@ -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.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 prints40: 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.@ -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 ofBLI_dir_create_recursive()
. Just useASSERT_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.@ -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.
@ -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.BLI: separate logic for changing working directoryto BLI: add function for changing working directory