search

ros-infrastructure / ros_buildfarm

ROS buildfarm based on Docker
Apache License 2.0
81 stars 96 forks source link

Make python3 interpreter replacement in scripts stricter #925

Closed cottsay closed 2 years ago

cottsay commented 2 years ago

There's a string replace in the generated bash scripts for various script jobs. It's there to ensure that the same python interpreter which was used to generate the script is used in the script's python invocations.

The current blind replacement is a little too aggressive and will replace any occurrence of 'python3 ' anywhere in the script, including URLs and package names. This change makes that replacement happen only at the beginning of a line or when there is whitespace before the invocation.

j-rivero commented 2 years ago

The current blind replacement is a little too aggressive and will replace any occurrence of 'python3 ' anywhere in the script, including URLs and package names. This change makes that replacement happen only at the beginning of a line.

Could be the case of having python3 invocations not strictly in the beginning of the line? Would make sense to include something like [[:space:]]python3[[:space:]] in the same regexp?

cottsay commented 2 years ago

Could be the case of having python3 invocations not strictly in the beginning of the line?

Maybe, but I don't think so. I know our CI coverage on this repo isn't perfect, but we do some jobs using Python 2, and I believe those would break if we used the wrong interpreter.

I'm happy to change it to include the possibility of leading whitespace, though.

nuclearsandwich commented 2 years ago

Are there any invocations like the below which this replacement would need to cover? I know that reprepro_updater and some dockerfile tasks use that style of invocation.

PYTHONPATH=mymodule:$PYTHONPATH python3 -m mymodule ...

Just in terms of template contents there seems to be some heavy usage

`rg -n 'PYTHONPATH=.*python3' | rg - v export` ``` ros_buildfarm/templates/release/release_create_trigger_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/release/release_create_reconfigure_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/devel/devel_create_tasks.Dockerfile.em:74: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/status/release_status_page_task.Dockerfile.em:45: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/release/release_check_sync_criteria_task.Dockerfile.em:46: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/status/blocked_source_entries_page_task.Dockerfile.em:45: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/status/blocked_releases_page_task.Dockerfile.em:45: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/devel/devel_task.Dockerfile.em:118: ' PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' ros_buildfarm/templates/devel/devel_create_reconfigure_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/release/rpm/binarypkg_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/rpm/binarypkg_task.Dockerfile.em:55: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/deb/binarypkg_create_task.Dockerfile.em:77: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/deb/binarypkg_create_task.Dockerfile.em:88: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/deb/binarypkg_create_task.Dockerfile.em:95: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/deb/sourcepkg_task.Dockerfile.em:63: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/release/deb/sourcepkg_task.Dockerfile.em:73: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/status/release_compare_page_task.Dockerfile.em:45: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/release/rpm/sourcepkg_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + ros_buildfarm/templates/ci/create_workspace.Dockerfile.em:97: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/ci/create_workspace.Dockerfile.em:105: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/ci/ci_create_tasks.Dockerfile.em:73: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/ci/ci_create_tasks.Dockerfile.em:83: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/ci/ci_create_tasks.Dockerfile.em:88: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/ci/ci_create_reconfigure_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/doc_metadata_task.Dockerfile.em:46:cmd = 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/doc_create_reconfigure_task.Dockerfile.em:47: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/doc_task.Dockerfile.em:96:cmd = 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/rosdoc2_create_task.Dockerfile.em:50: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/rosdoc2_task.Dockerfile.em:49:cmd = 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ros_buildfarm/templates/doc/doc_create_task.Dockerfile.em:59: 'PYTHONPATH=/tmp/ros_buildfarm:$PYTHONPATH python3 -u' + \ ```
cottsay commented 2 years ago

In the spirit of forward progress on this change, I've modified it to replace only when there is whitespace after the invocation and whitespace or a line beginning before it. I believe these are safe assumptions to make.

nuclearsandwich commented 2 years ago

In the spirit of forward progress on this change, I've modified it to replace only when there is whitespace after the invocation and whitespace or a line beginning before it. I believe these are safe assumptions to make.

Thanks. I think that's a sound investment into avoiding unintentional substitutions.