[for discussion, POC]: make loader arguments discoverable #1419

twiggler · 2025-12-02T19:54:59Z

No description provided.

codspeed-hq · 2025-12-02T20:02:15Z

Merging this PR will degrade performance by 17.71%

❌ 2 regressed benchmarks
✅ 9 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
❌	`test_benchmark[_data/loaders/acquire/test-windows-fs-c.zip-ZipLoader]`	1.9 ms	2.3 ms	-16.12%
❌	`test_benchmark[_data/loaders/acquire/test-windows-fs-c-relative.tar-TarLoader]`	1.8 ms	2.2 ms	-17.71%

_{Comparing poc-loader-args (beb1b9a) with main (4e9381b)}

twiggler · 2025-12-03T08:29:00Z

dissect/target/tools/query.py

+
+        # The loader now knows how to print its own help.
+        # We just need to instantiate it. Note that the path is not important here.
+        loader_instance = loader_cls(pathlib.Path())


Passing a dummy path is a bit dubious.

generate_argparse_for_plugin_class

dissect/target/tools/query.py

twiggler · 2025-12-17T10:31:06Z

dissect/target/loader.py

    def map(self, target: Target) -> None:
+        """Wrapper around the _map function that handles argument passing."""
+        # The loader now parses its own arguments
+        loader_options = self._parser.parse_args(self.loader_args)


Instead of rest arguments, use parse_known_args

twiggler · 2025-12-17T10:35:46Z

dissect/target/tools/utils/cli.py



-def open_targets(args: argparse.Namespace) -> Iterator[Target]:
+def open_targets(args: argparse.Namespace, loader_args: list[str] | None = None) -> Iterator[Target]:


Alternative: use loader namespace

Miauwkeru · 2026-01-12T13:31:19Z

dissect/target/loader.py

+        self, path: Path, *, parsed_path: urllib.parse.ParseResult | None = None, resolve: bool = True,
+        **kwargs


Suggested change

self, path: Path, *, parsed_path: urllib.parse.ParseResult | None = None, resolve: bool = True,

**kwargs

self, path: Path, *, parsed_path: urllib.parse.ParseResult | None = None, resolve: bool = True, **kwargs

Miauwkeru · 2026-01-12T13:34:15Z

dissect/target/target.py

-                elif not path.is_dir():
-                    raise TargetError(f"Failed to find loader for {path}: path is not a directory")

-                for entry in path.iterdir():
-                    for target in _open_all(entry, include_children=include_children):
-                        at_least_one_loaded = True
-                        yield target
+                if path.is_dir():
+                    for entry in path.iterdir():
+                        for target in _open_all(entry, include_children=include_children):
+                            at_least_one_loaded = True


why was this changed?

Miauwkeru · 2026-01-12T13:36:02Z

dissect/target/loader.py

+        # Pass the parsed options directly to the implementation map function
+        return self._map(target, **vars(loader_options))
+
+    def _map(self) -> None:


Suggested change

def _map(self) -> None:

def _map(self, **kwargs) -> None:

Miauwkeru · 2026-01-12T13:41:56Z

dissect/target/loaders/local.py

-        force_dirfs = "force-directory-fs" in self.parsed_query
-        fallback_to_dirfs = "fallback-to-directory-fs" in self.parsed_query


this would force this new method of doing these command line arguments where acquire will stop functioning properly as it uses these query parameters. https://github.com/fox-it/acquire/blob/c109fe4f5e642078931fc8c3d80ab7f39e1c496f/acquire/acquire.py#L2433

I think we can keep both approaches if we combine the force_directory_fs argument with the one from self.parsed_query.

However, I also think it is a bit weird to apply "stringified" arguments when using target from acquire. Perhaps we can get rid of this URI scheme altogether. The question then becomes how to pass arguments to the loader programmatically.

- Do not instantiate full instance of loader when displaying help - Process all known arguments

Miauwkeru · 2026-01-19T12:22:32Z

dissect/target/loader.py

+        # Pass the parsed options directly to the implementation map function
+        return self._map(target, **vars(loader_options))
+
+    def _map(self, **kwargs) -> None:


Suggested change

def _map(self, **kwargs) -> None:

def _map(self, target: Target, *, **kwargs) -> None:

After looking at it again, maybe like this instead as a target always gets passed down and we don't want any further positional arguments as far as I know

Miauwkeru · 2026-01-19T12:23:55Z

dissect/target/tools/utils/cli.py

            # Do not continue processing, we requested a child but got none
            raise

    return target


Probably a good idea to just remove your changes from this specific file

Miauwkeru · 2026-01-19T12:39:03Z

dissect/target/loader.py

+    def _create_parser(cls: type[Loader]) -> argparse.ArgumentParser:
+        """Creates the argument parser for this loader."""
+        parser = argparse.ArgumentParser(
+            prog=f"loader:{cls.__name__.lower()}",


It might be better to keep it in line with how the help is done for functions

E.g, like:

$ target-query -f activitiescache -h usage: target-query -f activitiescache [-h]

as using -h for the loader will now not give any information on how to use it with the tool:

$ target-query -L local -h usage: loader:localloader [--force-directory-fs] [--fallback-to-directory-fs]

which is a bit odd for a user perspective

Added comment, can be deferred when we implement it IMO

Miauwkeru · 2026-01-19T12:46:24Z

dissect/target/tools/query.py

+        if not args.loader:
+            parser.print_help()
+            return 0
+
+        if (loader_cls := LOADERS_BY_SCHEME.get(args.loader, None)) is None:
+            print(f"Error: Loader '{args.loader}' not found.")
+            return 1


moving this check to e.g. process generic arguments function in tools/utils/cli.py will add this behaviour to all target-* tooling instead of just query

We can do this in the proper implementation

Miauwkeru · 2026-01-19T12:48:18Z

dissect/target/loader.py

            dict(urllib.parse.parse_qsl(parsed_path.query, keep_blank_values=True)) if parsed_path else {}
        )

+        self._parser = self._create_parser(self.__class__)


still not quite sure we want to create the parser in the __init__ of the loader. As it feels (to me) that the argument parser should be created / handled before the loader is initiated.

I think the previous objection was that showing the help message required instantiation of the loader, which is no longer the case?

From an architectural standpoint, parsing arguments typically is done in a different (outermost) layer of a program. Is that the reason you want it done before the loader is initiated?

I don't necessarily disagree with that, but plugin arguments are also processed pretty "late":

dissect.target/dissect/target/tools/utils/cli.py

Line 424 in 95dba81

known_args, _ = parser.parse_known_args(args)

Moreover, originally the parsing of URI arguments was also done "late":
[target.parsed_path = urllib.parse.urlparse(str(target.path))](

dissect.target/dissect/target/loaders/local.py

Line 54 in f3bf586

target.parsed_path = urllib.parse.urlparse(str(target.path))

)

TLDR I think the poc is in line what is already there, while simultaneously agreeing it is architecturally unsound. If we change it, I propose we moving the parsing of arguments to the outermost layer of the program for all cases.

twiggler commented Dec 3, 2025

View reviewed changes

dissect/target/tools/query.py Show resolved Hide resolved

twiggler commented Dec 17, 2025

View reviewed changes

twiggler requested a review from Miauwkeru January 8, 2026 09:06

Miauwkeru requested changes Jan 12, 2026

View reviewed changes

twiggler added 2 commits January 19, 2026 10:21

let loader parse arguments

029742d

Process review comments:

40fb8be

- Do not instantiate full instance of loader when displaying help - Process all known arguments

twiggler force-pushed the poc-loader-args branch from c09be57 to f36dc28 Compare January 19, 2026 09:23

Fix erraneous changes to target

95dba81

twiggler force-pushed the poc-loader-args branch from f36dc28 to 95dba81 Compare January 19, 2026 09:27

Miauwkeru requested changes Jan 19, 2026

View reviewed changes

twiggler added 3 commits January 21, 2026 11:12

revert changes in main

ea03372

Add target as an argument

b36ba54

Add comment

beb1b9a



		def open_targets(args: argparse.Namespace) -> Iterator[Target]:
		def open_targets(args: argparse.Namespace, loader_args: list[str] \| None = None) -> Iterator[Target]:

		self, path: Path, *, parsed_path: urllib.parse.ParseResult \| None = None, resolve: bool = True,
		**kwargs

		force_dirfs = "force-directory-fs" in self.parsed_query
		fallback_to_dirfs = "fallback-to-directory-fs" in self.parsed_query

	def _map(self, **kwargs) -> None:
	def _map(self, target: Target, , *kwargs) -> None:

[for discussion, POC]: make loader arguments discoverable #1419

Are you sure you want to change the base?

[for discussion, POC]: make loader arguments discoverable #1419

Uh oh!

Conversation

twiggler commented Dec 2, 2025

Uh oh!

codspeed-hq bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 17.71%

Performance Changes

Uh oh!

twiggler Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

twiggler Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twiggler Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq bot commented Dec 2, 2025 •

edited

Loading

twiggler Dec 3, 2025 •

edited

Loading

twiggler Dec 17, 2025 •

edited

Loading

twiggler Jan 19, 2026 •

edited

Loading