refactor Core::new including command runner setup

[tdyas]

Problem

Core::new is a very convoluted function and it is hard to read what is actually going on in it.

Solution

Refactor the store and command runner setup into separate functions. (Also refactors the load of some certificates.) This refactor will make the subsequent introduction of remote caching in #10960 easier to comprehend.

Result

Existing tests pass.

[tdyas]

@gshuflin: I didn't end up introducing the enumeration that you had suggested. The parsing of the existing options seemed simple enough once the setup code was refactored out.

[gshuflin]

does this need to return a Result? I don't see a way this can fail

[tdyas]

I wanted to be consistent with the other refactored methods. They all return Result<_, String>. You are correct though, as it is currently, it cannot fail.

[illicitonion]

Let's not return a Result here - can always change the signature if we need to in the future :)

[tdyas]

Done. Now just Box<dyn CommandRunner>.

[coveralls]

Coverage remained the same at 0.0% when pulling c7b15b1 on tdyas:refactor_command_runner_setup into bf62b0a on pantsbuild:master.

[illicitonion]

Looks good :) Are you going to move CommandRunner to Session, too?

[illicitonion]

Let's not return a Result here - can always change the signature if we need to in the future :)

[tdyas]

Are you going to move CommandRunner to Session, too?

I was considering it, but probably not until after I have the remote caching PR finalized. This PR just cleans up Core::new enough to make the changes for remote caching easier to understand.

[stuhood]

Looks good :) Are you going to move CommandRunner to Session, too?

I'm not sure that that is a good idea... the Core is used across all runs, whereas a Session is recreated for each run. So to reduce latency, holding open a single top-level CommandRunner (and Store, etc) reduces per-run overhead.

The workunit store is provided thread/task-local, so per-Session workunits/metrics are recorded below the CommandRunner even if the runner is itself global.

[illicitonion]

Looks good :) Are you going to move CommandRunner to Session, too?

I'm not sure that that is a good idea... the Core is used across all runs, whereas a Session is recreated for each run. So to reduce latency, holding open a single top-level CommandRunner (and Store, etc) reduces per-run overhead.

The workunit store is provided thread/task-local, so per-Session workunits/metrics are recorded below the CommandRunner even if the runner is itself global.

My aim with the moving is to stop all of the remote execution related flags from being daemon-restarting flags... Ideally the flags would form part of the Graph, and the CommandRunner itself would form part of the graph, but that seems tricky... shrug something to think about, definitely not something urgent :)