-
Notifications
You must be signed in to change notification settings - Fork 75
Add support for --direct access for cim plugins #1437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
william-billaud
wants to merge
40
commits into
fox-it:main
Choose a base branch
from
william-billaud:cim_add_direct_support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
b5bcc7c
Add support for --direct access for cim plugins
william-billaud df955f1
Merge branch 'main' into cim_add_direct_support
william-billaud c36698b
Merge branch 'main' into cim_add_direct_support
william-billaud f183164
Merge branch 'main' into cim_add_direct_support
william-billaud 340ce17
Merge branch 'main' into cim_add_direct_support
william-billaud 1000f34
Merge branch 'main' into cim_add_direct_support
william-billaud db71419
Use case insentive vfs by default on direct mode.
william-billaud 28b5a4a
Fix test python < 3.12
william-billaud 5ea70ae
Merge branch 'main' into cim_add_direct_support
william-billaud 1afdbb9
Fix tests on case insensitive fs
william-billaud 25b8d17
Revert static version
william-billaud 4d60ab1
Use VirtualFilesystem (currently causing a deadlock)
william-billaud 6bc3c16
Merge branch 'main' into cim_add_direct_support
william-billaud 715c249
Fix direct loader vfs conversion to str, causing errors in tests
william-billaud 1e6a00d
Merge branch 'main' into cim_add_direct_support
william-billaud 6b076a4
Merge branch 'main' into cim_add_direct_support
william-billaud 5bc1b42
Revert tox ini change
william-billaud 076f512
Delete no longer used variable
william-billaud 21a6aee
Merge branch 'main' into cim_add_direct_support
william-billaud 4ed0bcd
Apply suggestion
william-billaud 867dbc0
Update tests/loaders/test_direct.py
william-billaud 8140f64
Add specific function to enumerate file for windows before 3.12
william-billaud 88cfc2f
Ruff
william-billaud 10f348b
Ruff
william-billaud 9f2a012
typo
william-billaud 880e20d
Add comment
william-billaud 4710aba
Merge branch 'main' into cim_add_direct_support
william-billaud 808e028
Merge branch 'main' into cim_add_direct_support
twiggler f1e01c4
Fix for python < 3.12 on windows
william-billaud 310632a
Lint
william-billaud 48719bd
Merge branch 'main' into cim_add_direct_support
william-billaud f413c9a
Update dissect/target/loaders/direct.py
william-billaud 8b2fd4a
Update dissect/target/loaders/direct.py
william-billaud f2d7ab1
Update dissect/target/tools/query.py
william-billaud 0a1efce
Update tests/tools/test_query.py
william-billaud 5815c12
Update dissect/target/target.py
william-billaud c674034
Move __repr__
william-billaud c6033b7
Fix docstyle
william-billaud 745e5d9
Fix open_direct call
william-billaud e9386bd
Merge branch 'main' into cim_add_direct_support
william-billaud File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Git LFS file not shown
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import logging | ||
| from pathlib import Path | ||
| from zipfile import ZipFile | ||
|
|
||
| import pytest | ||
|
|
||
| from dissect.target import Target | ||
| from tests._utils import absolute_path | ||
|
|
||
|
|
||
| def test_direct_overlap_warning( | ||
| tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Assert direct raise warning in case sensitive mode if some files overlap | ||
william-billaud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| We must uncompress files in a temporary directory as having two files with same name | ||
| would cause issue with git on case insensitive fs. | ||
| """ | ||
| ZipFile(absolute_path("_data/loaders/direct/overlap.zip")).extractall(tmp_path) | ||
| if len(list((tmp_path / "overlap").iterdir())) < 2: | ||
| pytest.skip("Test running on an insensitive fs") | ||
| with caplog.at_level(logging.WARNING): | ||
| _ = Target.open_direct([tmp_path], case_sensitive=True) | ||
| assert ( | ||
| "Direct mode used in case insensitive mode, but this will cause files overlap, " | ||
| "consider using --direct-sensitive" not in caplog.text | ||
| ) | ||
| _ = Target.open_direct([tmp_path], case_sensitive=False) | ||
| assert ( | ||
| "Direct mode used in case insensitive mode, but this will cause files overlap, " | ||
| "consider using --direct-sensitive" in caplog.text | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am considering if the case insensitivity check should not be an optional check in
VirtualDirectory::add.It should be more efficient and easier to implement there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar enough with these component but probably.
Nevertheless maybe this implementation could be considered a good enough for the --direct use case, and improvement could be managed in a dedicated issue (as it required more modification of internals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can defer to another ticket.
I will add a suggestion to make the overlap check more efficient tomorrow, along with another review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking at the implementation of
yield_all_file_recursivelyandcheck_case_insensitive _overlap. I noticed the comment mentioning that rglob isn't case-sensitive until Python 3.12. While that's a valid point to consider, the actual case-insensitive check in the current code happens when the paths are converted to lowercase.My proposed refactoring keeps this exact same logic but simplifies the first step of gathering the files. Instead of using a custom recursive function, we can use rglob to get all the file paths and then perform the same lowercase comparison. This works correctly on any OS, regardless of the filesystem's native case sensitivity, because the check is done in Python after collecting the files.
Here’s how we could refactor it:
(Untested)
What do you think of this approach?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's close to my initial approach.
unfortunately it does not work on with python <3.12 when target is on a case sensitive FS on windows. I was not able to catch it in test (it maybe related to the cpython implemention) using the Virtual Filesystem.
my comment is misleading as in fact this is not related to the case_sensitive parameter, but probably to some inner works when adding this option
Here is the output from a windows VM, where z: is virtualbox share with your suggestion and o has the following content
With your suggestion : Warning only raised in 3.12
With previous implementation : Warning raised with all version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented a logic to define a different function for this edge case.
This will allows to easily delete it in the (not near) future when support for python 3.11 will be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's unfortunate, there indeed appears to be a bug in python 3.11 on Windows (python/cpython#94537)
I think it is nice we can remove the workaround when python 3.11 support is dropped.