ARM-software / devlib

Library for interaction with and instrumentation of remote devices.
Apache License 2.0
45 stars 77 forks source link

Wipe target working directory upon disconnect #680

Open douglas-raillard-arm opened 3 months ago

douglas-raillard-arm commented 3 months ago

Following the discussion at https://github.com/ARM-software/devlib/pull/677#discussion_r1559807694:

Maybe it would make sense to default to cleaning up, and have WA override that default then ? In general, keeping resources deployed on the target are a recipe for trouble unless carefully implemented (e.g. checksuming the resources to ensure what is already deployed is actually what the code expects to work with). Even if WA does that carefully to avoid troubles, I wouldn't expect most client code to go to these length. In LISA, we push all our tools every time they are needed (at most once per Target instance, so it's "cached" but only very temporarily).

douglas-raillard-arm commented 3 months ago

Comment from @marcbonnici :

True, I agree it is safer in the general case to push to the target each time but even today a client can currently do so without issue. I would just question whether it makes sense for devlib to automatically handle the cleanup vs leave the decision to the client code itself via whatever mechanism they choose.

So far we haven't placed any restrictions on the expectations on the working directory, so I'd be hesitant to introduce a new default that would wipe it out as we can't say how other users / client code could be using this directory.

Essentially I think I might be missing what the benefit would be to the end user vs the potential downsides of clearing files the user may not have wanted to?

douglas-raillard-arm commented 3 months ago

So far we haven't placed any restrictions on the expectations on the working directory, so I'd be hesitant to introduce a new default that would wipe it out as we can't say how other users / client code could be using this directory.

Yeah, in that case maybe the default should stick to current behavior, and we can simply add an option to do so.

Essentially I think I might be missing what the benefit would be to the end user vs the potential downsides of clearing files the user may not have wanted to?

Things can pile up, and it's not desirable. AFAIR things like trace.dat will stay there "forever". In our Linux targets, we typically have a buildroot rootfs over tmpfs, so having big files stay there just clogs memory for subsequent experiments until reboot. Running tests on the my development machine leaves stuff behind in a non-standard location, which I'm not super fond of. Generally speaking, I'd expect something like devlib to leave things they way it found it in cases where it could achieve an orderly exit. That's a big part of the push to using context managers for setting things like CPU frequency.