Skip to content

WIP feat(client): Isolate client directory and global directory - #2336

Draft
agajdosi wants to merge 1 commit into
BlenderKit:mainfrom
agajdosi:new_client_home
Draft

agajdosi wants to merge 1 commit into
BlenderKit:mainfrom
agajdosi:new_client_home

Conversation

@agajdosi

@agajdosi agajdosi commented Oct 8, 2026

Copy link
Copy Markdown
Member

bude ještě potřeba syncnout na straně clienta

@agajdosi
agajdosi marked this pull request as draft October 8, 2026 08:41
@codecov

codecov Bot commented Oct 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.82609% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.30%. Comparing base (4486ac7) to head (13f50b0).

Files with missing lines Patch % Lines
client_lib.py 56.25% 7 Missing ⚠️
ui_panels.py 0.00% 2 Missing ⚠️
utils.py 33.33% 2 Missing ⚠️
timer.py 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2336   +/-   ##
=======================================
  Coverage   34.30%   34.30%           
=======================================
  Files          87       87           
  Lines       27669    27683   +14     
=======================================
+ Hits         9491     9497    +6     
- Misses      18178    18186    +8     
Flag Coverage Δ
python 34.30% <47.82%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
timer.py 43.44% <50.00%> (-0.13%) ⬇️
ui_panels.py 23.30% <0.00%> (ø)
utils.py 28.60% <33.33%> (ø)
client_lib.py 59.48% <56.25%> (-0.39%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tweekazoid

Copy link
Copy Markdown
Contributor

udelej na clientovi taky PR,
to se mergne jako prvni, a tady budes uz pracovat jen s "main" clienta

az budes ready

@Tweekazoid

Copy link
Copy Markdown
Contributor

Review: Isolate Blendkit-Client directory from Global Directory

Client PR (branch 67_new_client_dir) + add-on PR #2336, fixes #2335

Summary

The goal is good. Moving the client's binaries, log and system_id out of global_dir into a per-user app-data folder means the client no longer depends on a folder the user can change, put on a network or read-only drive, or delete. After this, global_dir holds only assets.

Before After
Client binaries <global_dir>/client/bin/vX.Y.Z/ <client_dir>/bin/vX.Y.Z/
Client log <global_dir>/client/default.log <client_dir>/default.log
system_id <global_dir>/client/system_id <client_dir>/system_id
Client config / settings JSON next to the binary next to the binary, so they move with it

<client_dir> = %LOCALAPPDATA%\blendkit_client on Windows, $XDG_DATA_HOME/blendkit_client (falling back to ~/.local/share/blendkit_client) elsewhere.

As written, though, the two PRs break the released add-ons, don't work as intended on Windows, and split the machine ID between the add-on and the client. Details below.


Client PR: blocking

1. Removing the --global_dir flag breaks every released add-on (this is the CI failure).
client_lib.start_blenderkit_client() in released add-ons (and in #2336) still passes --global_dir <path>. Go's flag.Parse() uses ExitOnError, so the client exits with code 2 straight away. Nothing listens on the port, and every add-on test fails with ConnectionRefusedError: [Errno 111] Connection refused (e.g. Test04GetReportsClientRunning.test_get_reports_another_app_id).
Please keep accepting the flag, even if it's ignored:
_ = flag.String("global_dir", "", "DEPRECATED, ignored")
Rule of thumb: never remove a CLI flag the add-on passes; deprecate it and ignore it. The client is merged first and has to work with already-released add-ons.

2. The Windows branch never runs: runtime.GOOS == "Windows".
runtime.GOOS is lowercase ("windows"), so on Windows the client falls through to the XDG path and uses C:\Users\<user>\.local\share\blendkit_client. The add-on (#2336) uses %LOCALAPPDATA%\blendkit_client, so the two disagree on where the client's files live on Windows.

3. path.Join is used for filesystem paths in clientDirectoryPath().
path is for slash-separated paths (URLs). Please use filepath.Join, like the rest of the file.

4. Missing LOCALAPPDATA isn't handled.
Once (2) is fixed, an empty LOCALAPPDATA gives a relative blendkit_client path, and the ID gets written to the current working directory. Return "" in that case so the existing "no home" handling kicks in.

Client PR: system_id migration / data loss

5. Migration from IDs written by client 1.13.5 / 1.13.6 was removed.
main read these as legacy sources: <global_dir>/client/system_id, <global_dir>/system_id, ~/blenderkit_data/client/system_id and ~/blenderkit_data/system_id. The new resolveSystemID only reads ~/blenderkit_data/system_id.
Users whose ID was first created by 1.13.5/1.13.6 (new installs since then) will get a fresh MAC-derived ID. It usually matches the old one, but not when the MAC changed (randomization, VPN, adapter swap), which is exactly the case persisting the ID was meant to cover.
Please add at least ~/blenderkit_data/client/system_id to the legacy candidates. The --global_dir value from (1) could be used for migration only, to find <custom_global_dir>/client/system_id.

6. Don't delete the legacy ~/blenderkit_data/system_id.
It's the last fallback in the add-on's paths.get_stable_system_id(). Older add-on/client pairs, e.g. in another Blender version on the same machine, also still read and write it. Deleting it pushes them back to a fresh MAC-derived ID or Python's uuid.getnode(). main deliberately copied without deleting; please keep it that way.

Client PR: tests

7. Typos in the scenario names mean the corrupt-file cases are never tested.
In TestResolveSystemIDConfiguredPath:

  • the scenario list has "legacy corrupt", but the branches check if scenario == "corrupt legacy", so the corrupt content is never written;
  • "persisted corrupt " has a trailing space, so scenario == "persisted corrupt" never matches;
  • the persisted branch also checks "corrupt legacy" (copy-paste) instead of "persisted corrupt".

8. The tests aren't isolated on Windows.
They only set XDG_DATA_HOME. Once (2) is fixed, they'll read and write the developer's real %LOCALAPPDATA%\blendkit_client\system_id and leak state between tests. Please also t.Setenv("LOCALAPPDATA", dir) in every system-id test (and "" in TestPersistedSystemIDWithoutHome).

9. TestResolveSystemIDUnwritableDataDir blocks the wrong folder.
It creates a file named blenderkit_data to make MkdirAll fail, but the data folder is now blendkit_client. The test passes without exercising the unwritable path.

10. Leftover debug print: fmt.Println(os.ReadFile(expectedIDFilepath)) in TestResolveSystemIDConfiguredPath.


Add-on PR #2336

11. The add-on and client disagree on where system_id lives (most important on this side).
paths.get_system_id_filepath() is unchanged and still returns <global_dir>/client/system_id, but the client now writes <client_dir>/system_id.

  • Existing users: the add-on keeps reading an old copy. Same value, so nothing breaks yet.
  • New installs: no file exists where the add-on looks, so it falls back to uuid.getnode(). Per the docstring in get_stable_system_id(), that picks a different adapter than the client on ~79% of Windows machines, so one machine reports two IDs.
    Please point get_system_id_filepath() at get_client_directory()/system_id and keep the old paths as read-only fallbacks.

12. get_client_directory() can crash on Windows.
path.join(os.getenv('LOCALAPPDATA'), dirname) raises TypeError when the variable is missing (seen in some sandboxed/service setups). Please guard it, e.g. fall back to Path.home() / "AppData" / "Local".

13. check_clientdir_permissions() only logs.
If <client_dir> isn't writable, nothing in the startup flow reacts. The in-place fallback is triggered later in ensure_client_binary_installed() / the log open, which is fine. But is the user supposed to see the message? Currently it's just bk_logger.error.

14. --global_dir is still passed to the client.
That's fine (and required for older clients), but please confirm it's intentional. If the client keeps it only for migration (see 5), a short comment in start_blenderkit_client() would help.


Impact / rollout

Add-on Client Result
released (passes --global_dir) this client PR as-is broken: client exits on the unknown flag
released client PR with (1) fixed works
#2336 old client works (old client accepts --global_dir)
#2336 client PR with (1)(2) fixed works, but system_id is split until (11) is fixed

Other effects to be aware of:

  • Old binaries and logs in <global_dir>/client/bin/v* and <global_dir>/client/*.log are never cleaned up. Harmless, but it uses disk space and may confuse support when users send logs.
  • The client config/settings JSON sits next to the binary, so the first start in <client_dir> uses fresh files. Please check that the shared usage-data opt-out isn't pushed back to the add-on as the default (sharing allowed) on that first start.
  • Merge order: the client PR first (it must stay backward compatible with released add-ons), then WIP feat(client): Isolate client directory and global directory #2336 pinned to the new client version.

Suggested checklist before merge

  • Client: keep --global_dir (ignored, or used only for migration)
  • Client: runtime.GOOS == "windows", filepath.Join, guard an empty LOCALAPPDATA
  • Client: restore migration from ~/blenderkit_data/client/system_id (and <global_dir>/client/system_id)
  • Client: copy the legacy system_id, don't delete it
  • Client tests: fix scenario-name typos, set LOCALAPPDATA in tests, fix the unwritable-dir test, remove the debug print
  • Add-on: point get_system_id_filepath() at the new client dir, keep the old paths as fallbacks
  • Add-on: guard os.getenv('LOCALAPPDATA') being None
  • Re-run the add-on test matrix against the new client build

@Tweekazoid Tweekazoid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bohuzel kdyby se to ted mergnulo tak to zpusobi dost komplikaci.

Je treba tam zafixovat par veci

@agajdosi

agajdosi commented Nov 6, 2026

Copy link
Copy Markdown
Member Author
  1. Removing the --global_dir flag breaks every released add-on (this is the CI failure).

TBH BS
there is no need to persist global-dir flag
older versions of add-on start their own pinned older versions of client which they ship in their zips, they never touch newer versions of client.
CLI is not API.

  1. The Windows branch never runs: runtime.GOOS == "Windows".

good catch

  1. path.Join is used for filesystem paths in clientDirectoryPath().

yep, fixed

  1. Missing LOCALAPPDATA isn't handled.

this gonna be rare, but I have added a fallback to %USERPROFILE%/.Local/AppData

  1. Migration from IDs written by client 1.13.5 / 1.13.6 was removed.

imho enough to get the location from few last gold releases, no need to cover edgy alphas for few tens of users
especially when MAC will be the same for 90% of them

  1. Don't delete the legacy ~/blenderkit_data/system_id.

ok

  1. Typos in the scenario names mean the corrupt-file cases are never tested.

ok

  1. The tests aren't isolated on Windows.

was there from before, seems like nobody runs the tests on windows
added set.env LOCALAPPDATA

  1. TestResolveSystemIDUnwritableDataDir blocks the wrong folder.

Problem is somewhere else... this was actually false positive test from before.
Fixed it now.

  1. Leftover debug print: fmt.Println(os.ReadFile(expectedIDFilepath)) in TestResolveSystemIDConfiguredPath.

a draft right...

  1. The add-on and client disagree on where system_id lives (most important on this side).
    paths.get_system_id_filepath() is unchanged and still returns <global_dir>/client/system_id, but the client now writes <client_dir>/system_id.

Bullshit. ofc get_system_id_filepath() did not change.
the underlying get_client_directory has changed.

def get_system_id_filepath() -> str:
    """Where Blendkit-Client keeps the machine ID; the add-on only reads it.
    Mirrors bk_client:utils.go-systemIDFilePath()."""
    client_dir = client_lib.get_client_directory()
    return os.path.join(client_dir, "system_id")
  1. get_client_directory() can crash on Windows.

lets add a fallback of %USERPROFILE%/.Local/AppData

  1. check_clientdir_permissions() only logs.
  2. --global_dir is still passed to the client.

todo on a draft

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Isolate Blendkit Client into a separate directory

2 participants