-
Notifications
You must be signed in to change notification settings - Fork 173
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
Can the ROS1 dockerfiles provide ROS_PYTHON_VERSION?+ #409
Comments
We do try not to duplicate the definition of environment variable in the Dockerfiles so we set only the one required to build the image itself. So we do not set any ROS env variable other than ROS_DISTRO. On the other hand we were discussing the necessity of retrieving the ROS_VERSION and ROS_PYTHON_VERSION from the rosdistro index. So once we have them it shouldn't cost much to set them in the dockerfile as these are not meant to change for the lifetime of the ROS distribution. |
True, it's becoming common to source the setup.sh file too before invoking commands that expect a sourced environment.
Yea, I can't think of a common use case that would need to change it or have it unset. |
Then it may get more misleading: This is all the ROS stuff set in the environment for noetic as a reference:
|
Yea, that make me lean toward just suggesting fokes to source the setup, as setting all those paths and default ENVs could screw with sourcing different setup files, like ros1_bridge, or when using multiple workspace overlays. |
I've always just prefixed commands in containers with It's basically the same as the entrypoint I believe, but also works with workspaces. |
👍 yes this is what I use too. I believe what ruffsl meant by
Was that if users in general end up "often/always prefixing commands with it" (which seems to be your case) it may be a reason for providing an already set up environment in the container. But as pointed above it may be too intrusive so documenting this alternative for users will be the way to go. Another thing we could consider: provide a utility command/alias to keep it less verbose and error-prone. (there is often reports of people trying to source and failing because source is a bash command). I could imagine something called rosenv that people could use in dockerfile
|
Well, such a utility could help make Dockerfile directives and instructions more shell agnostic, but how would in know which path setup source to replicate in the environment? Simply prefixing ros related directives using |
I would prefer, if you would name differently..
IMHO this is the best approach. It states clearly which environment is used. |
What's the practical difference between |
Do you know if there is an upstream ticket for this (or know of a reason for that script not being present in ROS 2/colcon/ament packages) ? It would be a valuable addition
IIRC The setup.sh contains all the logic for the environment setup (and is meant to be sourced) and has lasting effect on the current environment, the other is a script sourcing the former and executing the command passed as an argument (does not have lasting effect, setup the env only for the one command.
These are not:
haha I see. Thinking about it more, there is little value in providing such an alias and educating people to use the scripts present in the install space is a better way to go. As said above it also "states clearly which environment is used" instead of hiding it from users. |
Nice, that's a really clean way |
Looks like we're in agreement that the install scripts should be used to setup the environment within the RUN directive, and that dockerfiles shouldn't hardcode the ROS_PYTHON_VERSION evn itself. With the update to the library repo documentation, I think we could close this. |
Yes, thanks for the discussion |
I'm migrating to noetic. On my system I can use
$ROS_PYTHON_VERSION
as this is provided by theros_environment
package. But during the build of a Dockerfile this variable is not provided, only after sourcing of the workspace.Can you provide
$ROS_PYTHON_VERSION
during the build? I'm willing to contribute a PR for this.As a workaround I'm using
The text was updated successfully, but these errors were encountered: