Repository navigation
Fix BackgroundCommand.send_signal() and add trace-cmd record support - #689
Conversation
douglas-raillard-arm
changed the title
|
Upon deeper inspection, the |
9696cdc to
8d55c17
Compare
|
PR rebased |
8d55c17 to
8fb315f
Compare
|
@marcbonnici Updated with rework of temp folder management, so it works on old versions of android as well. I can split that in a separate PR if needed but left it on top for now to ease testing of the combination. |
8fb315f to
852f2d6
Compare
852f2d6 to
765d6b7
Compare
|
Updated with a fix to |
|
Hi @douglas-raillard-arm Apologies for the radio silence on this. |
|
@marcbonnici The last thing needed was to adjust the doc, but I can't find any documentation for FtraceCollector so we might just handle that separately. |
|
We havent done any dog-fooding on the commits related to temp file management, unlike on the rest of the PR, so maybe it's worth having a second look at that part |
douglas-raillard-arm
changed the title
|
Added the branch to our vendored version for dogfooding |
| connection_settings=None, | ||
| platform=None, | ||
| working_directory=None, | ||
| *, |
There was a problem hiding this comment.
With hindsight I agree this seems like it would be beneficial, however I'm concerned this could be a breaking change for existing setups.
IIRC we've largely managed to keep the current parameters consistent so far however maybe we should add this restriction for any new parameters going forward?
There was a problem hiding this comment.
I was a bit undecided about this one. If we don't do it now, the order will basically be random as as tmp_directory will have to be placed at the end of the list after max_async, far away from executables_directory and working_directory.
Considering the fix in user code is trivial and fully backward compatible for older devlib versions, I don't think it's so bad.
This also paves the way to getting rid of the copy pasta soup of parameters in all the Target subclasses. I realized I missed the ChromeOsTarget one.
There was a problem hiding this comment.
If we don't do it now, the order will basically be random as as tmp_directory will have to be placed at the end of the list after max_async, far away from executables_directory and working_directory.
That is true, however from devlib's perspective I sadly I think the other parameters are already essentially random as we've been following this rule so far.
From the user's perspective if a user is already using keyword arguments (as I guess we hope people are) then they are free to organize their parameters as they like. However if they are not, then we break their code for this one parameter and although the fix is trivial I'm not sure the benefit would be worth it.
If we were to make tmp_directory keyword only we should at least be able to prevent this problem going forward.
What do you reckon?
There was a problem hiding this comment.
True I guess that ship has sailed. I'll change the PR to be "append only" on the parameter list and I'll open an issue to leave the keyword-only part to the future.
If we are happy to have an *args, **kwargs signature, we would be able to implement a deprecation warning to users using any positional parameters for a while before pulling the plug.
765d6b7 to
849a88c
Compare
|
PR updated with fixes for comments and an extra commit at the top to replace most |
Support sending any signal to background commands, instead of only
supporting properly SIGKILL/SIGTERM/SIGQUIT.
The main issue is figuring out what PID to send the signal to, as the
devlib API allows running a whole snippet of shell script that typically
is wrapped under many layers of sh -c and sudo calls. In order to lift
the ambiguity, the user has access to a "devlib-signal-target" command
that points devlib at what process should be the target of signals:
# Run a "setup" command, then the main command that will receive the
# signals
cmd = 'echo setup; devlib-signal-target echo hello world'
with target.background(cmd) as bg:
bg.communicate()
The devlib-signal-target script can only be used once per background
command, so that it is never ambiguous what process is targeted, and so
that the Python code can cache the target PID. Subsequent invocations
of devlib-signal-target will fail.
Allow using trace-cmd record that continuously dump the trace to disk, allowing to overcome the buffer size limitations when recording for extended periods of time.
* Make use of Target.make_temp() in Target._xfer_cache_path() to
deduplicate temp folder creation code
* Introduce Target.tmp_directory attribute for the root of temporary
files.
* Make Target.tempfile() use Target.tmp_directory
* Rework the Target._resolve_paths() implementations:
* Target.tmp_directory is set to a default returned by "mktemp -d".
This way, if "mktemp -d" works out of of the box, all devlib
temporary files will be located in the expected location for the
that operating system.
* Target._resolve_paths() must set Target.working_directory and that
is checked with an assert. Other directories have defaults based
on this if _resolve_paths() does not set them.
Also avoid a "None" prefix when no prefix is asked for, and set None as the default prefix value. Remove the "devlib-test" default value as make_temp() has nothing to do with tests.
Align the parameters between the 2 methods: * Use "None" as default value * Do not add suffix or prefix if not asked for. * Separate components with "-" instead of "_"
Replace as many uses of tempfile() by make_temp() as possible, as the latter provide more reliable resource control by way of a context manager. This also paves the way to having a single point in devlib where temporary files are created, simplifying maintenance.
849a88c to
982a59e
Compare
|
PR updated with removal of keyword-only patch and tmp_directory appended at the end of all parameter lists. |
3 participants
Fixes #645
Supersedes #643
TODO:
Closes #643