Kill existing processes in binary context managers#3581
Kill existing processes in binary context managers#3581Apeiros-46B wants to merge 3 commits intoUBC-Thunderbots:masterfrom
Conversation
Apeiros-46B
requested review from
Andrewyx,
sauravbanna and
williamckha
as code owners
williamckha
requested review from
a team,
GrayHoang and
itsarune
and removed request for
a team
| :param enable_realism: a argument (--enable_realism) that is going to be passed to er_force_simulator_main binary | ||
| """ | ||
| self.simulator_runtime_dir = simulator_runtime_dir | ||
| self.generic_command = [ |
There was a problem hiding this comment.
should we have this defined twice across two separate files, or declare some constant that can be imported?
There was a problem hiding this comment.
For something like this I'm fine with repeating ourselves
nycrat
left a comment
nycrat
left a comment
There was a problem hiding this comment.
Mostly looks good, there are some easy to fix issues with the full_system binary context manager.
I'll also suggest testing that the functionality of running full_system in debug and sudo works because when I tested it, the new is_cmd_running util function seems to not recognize the running processes. Perhaps it's cause I'm on mac; I'll test on ubuntu and give an update when I can
| if not self.debug_full_system: | ||
| kill_cmd_if_running(self.generic_command) |
There was a problem hiding this comment.
Should also check if self.should_run_under_sudo, since that also requires that full_system is running separately. Or just put this line in the else statement later on like you did in simulator.py
| self.generic_command = [ | ||
| # We keep the path relative to match processes that might have been | ||
| # started in different working directories, but keep the runtime dir | ||
| # the same as this one so we don't kill other fullsystems | ||
| "unix_full_system", | ||
| "--runtime_dir={}".format(self.full_system_runtime_dir), | ||
| ] |
There was a problem hiding this comment.
Since we implemented using different runtimes for the previous RFC, the name for the process might not be "unix_full_system", should probably use the path_to_binary here instead.
4 participants
Description
Implement a helper function to kill existing processes before starting the new instance in binary context managers
Testing Done
(not quite sure how to test this to be honest)
Resolved Issues
Resolves #3342
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue