osrf / rocker

A tool to run docker containers with overlays and convenient options for things like GUIs etc.
Apache License 2.0
561 stars 73 forks source link

Allow regular expression device strings #112

Closed peterpolidoro closed 3 years ago

peterpolidoro commented 3 years ago

It might be nice to allow regular expressions when specifying devices, including all values that match the regular expression. I do not know if such a feature is within the scope of this package, but I have found it to be very useful with multiple devices.

For example, instead of writing this:

rocker test_image --devices /dev/dri/card0 /dev/ttyACM0 /dev/ttyACM1 /dev/ttyACM2 /dev/ttyACM3 /dev/ttyACM4 /dev/ttyACM5

You could instead write something like this:

rocker test_image --devices /dev/dri/card0 /dev/ttyACM[0-5]

Or even something like this to connect 16 devices:

rocker test_image --devices /dev/dri/card0 "/dev/ttyACM([0-9]|1[0-5])"

The code in this pull request seems to work fine, but it is just a suggestion for one way it could be implemented. It uses sre_yield, which adds another dependency unfortunately, but that makes the regular expression matching easy. There may be a much cleaner location for the matching of the regular expression.

I removed the block of code that added devices to the docker_args in core.py. It seemed redundant when using the devices extension. Perhaps that second check is necessary after all if the devices extension is optional, but then it should probably only be performed if the devices extension is not used.

Other arguments, like volumes, may benefit from a regular expression expansion also, perhaps the expansion could happen in a more centralized location so it acts on every argument where it makes sense without needing to be repeated.

codecov[bot] commented 3 years ago

Codecov Report

Merging #112 (144c796) into master (4b81302) will decrease coverage by 0.08%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   86.89%   86.80%   -0.09%     
==========================================
  Files           7        7              
  Lines         595      591       -4     
==========================================
- Hits          517      513       -4     
  Misses         78       78              
Impacted Files Coverage Δ
src/rocker/core.py 90.56% <ø> (-0.31%) :arrow_down:
src/rocker/extensions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b81302...144c796. Read the comment docs.

peterpolidoro commented 3 years ago

Yes I was thinking of scripting the interface, using udev rules to trigger containers to restart with new device permissions when devices are connected or disconnected from the system. Good point, though, that any extra dependencies should be on my script's side rather than in this tool, since they are not needed when calling from bash. I will close this thanks.