Skip to content

Fix kick off #624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
marcbonnici merged 2 commits into from
Apr 29, 2023
Merged

Fix kick off #624

marcbonnici merged 2 commits into from
Apr 29, 2023

Conversation

@douglas-raillard-arm
Copy link
Collaborator

@douglas-raillard-arm douglas-raillard-arm commented Apr 4, 2023

No description provided.

@douglas-raillard-arm
shutils: Fix broken redirections
1f7ebd9 
Redirecting all output to /dev/null needs >/dev/null 2>&1 .

Fix cases where 2>&1 /dev/null was used, and also remove &> that is not
POSIX.
@douglas-raillard-arm
target: Fix and generalize Target.kick_off()
6877f03 
kick_off() implementation had a number of issue:
* Target specific implementation making testing more difficult.
* Not wrapping the command in sh -c lead to blocking behavior if the
  command had multiple parts, e.g. "sleep 42; echo foo"
* nohup sometimes writes to stdout, breaking return code parsing in
  adb_shell().

These issues are fixed by a generic implementing of kick_off() that
simply delegates to Target.background().

Fixes ARM-software#623
@douglas-raillard-arm
Copy link
Collaborator Author

douglas-raillard-arm commented Apr 6, 2023

Following @mrkajetanp tests, I updated the PR to use Target.background() instead of relying on nohup and &. The & unfortunately only makes the command a background job in the shell, but adb shell blocks until all background jobs are finished before exiting, making it useless.

Instead, using Target.background() provides the expected behavior. The main change is that background commands are canceled on target disconnect for some connection classes (but not all it seems, we might want to align behaviors there)

@douglas-raillard-arm
Copy link
Collaborator Author

douglas-raillard-arm commented Apr 6, 2023 •
edited
Loading

So we should probably:

  • deprecate Target.kick_off() as it really is just a Target.background() that leaks which is definitively an anti-pattern
  • Align all connection classes to cancel running background commands when the connection is closed. This can be done in generic code.

EDIT: the 2) is implemented there but needs an adjustment before being ready:
#626

@douglas-raillard-arm douglas-raillard-arm mentioned this pull request Apr 6, 2023
Merged
@mrkajetanp
Copy link
Contributor

mrkajetanp commented Apr 11, 2023

I can confirm that with the updated PR the instrument finally works as expected so simply getting rid of kick_off this way is probably a good idea.

marcbonnici
marcbonnici approved these changes Apr 29, 2023
View reviewed changes
Copy link
Collaborator

@marcbonnici marcbonnici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for some context, the original intention behind the kick_off method was to support platforms where having multiple connections was not possible (e.g. gem5) and therefore needed to be different to the regular background command.

With all the new changes that have bee introduced this is not a configuration that is expected, however in theory it would be possible for a connection implementation to override the kick_off mechanism to try and accommodate such requirements.

@marcbonnici marcbonnici merged commit 27fb045 into ARM-software:master Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcbonnici marcbonnici marcbonnici approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@douglas-raillard-arm @mrkajetanp @marcbonnici