Skip to content
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.

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

Don't use FileType for param-file #351

Merged

Conversation

jordan-palacios
Copy link
Member

As it is, the spawner.py crashes when reading the param_file argument since it is of type FileType instead of string.

See the following traceback:

Traceback (most recent call last):
  File "/home/user/exchange/tiago_ws/install/controller_manager/lib/controller_manager/spawner.py", line 141, in <module>
    sys.exit(main())
  File "/home/user/exchange/tiago_ws/install/controller_manager/lib/controller_manager/spawner.py", line 101, in main
    ret = subprocess.run(['ros2', 'param', 'load', controller_name,
  File "/usr/lib/python3.8/subprocess.py", line 489, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1637, in _execute_child
    self.pid = _posixsubprocess.fork_exec(
TypeError: expected str, bytes or os.PathLike object, not _io.TextIOWrapper

FileType is used for opening the selected file and returns a handle to it. The caller is also responsible for closing the handle later.

This is not the intention of the spawner.py script since we only wish to pass the specified file path to the ros2 param load command.

FileType tries to opens the selected file and returns a handle to it.
This is not the intention of the spawner.py script.
@Karsten1987
Copy link
Contributor

I actually think that FileType is the right thing to do here as it directly checks whether the file is available or not.
Couldn't we change the line below as such?

param_file = args.param_file.name

@jordan-palacios
Copy link
Member Author

That would work too. But as I said, FileType also give you a file handle you have to manage. And that does not really make sense since we don't want to access the file at all.

The script ends up calling ros2 param load which already checks for file existence (and reports the proper error). If that is not enough we can add a simple os.path.exists(path) here and be done with it.

@Karsten1987
Copy link
Contributor

os.path.exists(path) would make sense to me!

@jordan-palacios
Copy link
Member Author

os.path.exists(path) would make sense to me!

Done!

Failing CI job does not seem to be related to this PR.

@Karsten1987 Karsten1987 merged commit eb13322 into ros-controls:master Apr 6, 2021
@jordan-palacios jordan-palacios deleted the spawner_param_file_arg_fix branch April 6, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants