Handle recent changes to Fuchsia's ffx tool #6112
Conversation
ffx emu now needs to know the locations of some host tools. Copy these paths from the default ffx config into the configuration for the isolated ffx instance that syzkaller uses for most tasks. Also refactors the ffx helpers to make the options more visible for each command.
ffx log is now built separately from the main ffx binary.
|
|
||
| type ffxCommandOpts struct { | ||
| isolated bool | ||
| startDaemon bool |
There was a problem hiding this comment.
Nit: Given that there is only one location where startDaemon is true, maybe we can just run ffx daemon start -b as a separate call in that location? Or, alternatively, I think we can define startDaemon as !isolated because isolated is the only case we wanted to suppress it, I believe, and it is a no-op if already running for the other cases. Then we could avoid having this extra struct.
I am torn between the benefit of having names for the boolean args like this, and the added complexity, so defer to your judgement for the final call.
| func (inst *instance) ffxCommand(args ...string) *exec.Cmd { | ||
| cmd := osutil.Command(inst.ffxBinary, args...) | ||
| func (inst *instance) ffxCommand(opts ffxCommandOpts, binary string, args ...string) *exec.Cmd { | ||
| config := []string{"-c", "log.enabled=false,ffx.analytics.disabled=true"} |
There was a problem hiding this comment.
Nit: We previously only set these options explicitly when running non-isolated, so just want to make sure I understand the consequences of moving this here.
I believe running isolated implies ffx.analytics.disabled=true, so this shouldn't affect that behavior. For logging, though, do we want to disable that in the isolated case or might it be good to keep for debugging? If I recall correctly, we disabled it in the non-isolated case because it wanted to write to the home directory or some other place that potentially wouldn't have write access.
| @@ -340,18 +364,30 @@ func (inst *instance) connect() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (inst *instance) ffxCommand(args ...string) *exec.Cmd { | |||
| cmd := osutil.Command(inst.ffxBinary, args...) | |||
| func (inst *instance) ffxCommand(opts ffxCommandOpts, binary string, args ...string) *exec.Cmd { | |||
There was a problem hiding this comment.
Are all ffx binaries compatible with the same base -c ... config arguments?
| @@ -271,8 +290,10 @@ func (inst *instance) startSshdAndConnect() error { | |||
| if inst.debug { | |||
| log.Logf(1, "instance %s: started alpine container", inst.name) | |||
| } | |||
| opts.startDaemon = false | |||
There was a problem hiding this comment.
Nit: Even though it takes more space, I think it would be easier to read to have a whole new ffxCommandsOpts instead of having to track mutation
| @@ -229,8 +234,10 @@ func (inst *instance) Close() error { | |||
| } | |||
|
|
|||
| func (inst *instance) startFuchsiaVM() error { | |||
| inst.runFfx(30*time.Second, "config", "get", "product.path") | |||
There was a problem hiding this comment.
Was this doing something important? I have no idea why this was here 😆
| if err := cmd.Start(); err != nil { | ||
| if inst.debug { | ||
| log.Logf(0, "instance %s: failed to start ffx log", inst.name) |
There was a problem hiding this comment.
Nit: Might as well log err too?
Recently, some subcommands of
ffxhave b 8000 een split into separate binaries, and others depend on prebuilt artifacts. This PR updates the Starnix VM startup script to get the current path for each artifact (either from the default ffx config or from the tool manifest), and refactors the ffx helpers to make the call sites more consistent.(Starnix bug ref: this is a fix for https://fxbug.dev/409390594.)