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

Set ROS_DISTRO for rosdoc2 to enable link to repository #1078

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rkent
Copy link
Contributor

@rkent rkent commented Dec 11, 2024

rosdoc2 added a feature to automatically generate a link to the package repository in ros-infrastructure/rosdoc2#161 but it relies on the environment variable ROS_DISTRO being available at runtime. This PR sets that for buildfarm runs to enable this feature.

This has been tested locally using generate_doc_script but not on a buildfarm run.

@cottsay
Copy link
Member

cottsay commented Dec 11, 2024

Thanks for the PR.

The buildfarm configuration documentation says that build_environment_variables is supported for all build file types, which includes doc build configs. It appears that this value is left unused in the code, which could certainly be confusing.

I have a few independent questions, mostly directed at @nuclearsandwich and @tfoote:

  1. Would it be appropriate to plumb build_environment_variables into doc jobs? It'd be simple to do.
  2. If not, should we fix the configuration files so that value is no longer accepted in valid doc configs?
  3. If yes, would it be more appropriate to use that mechanism to specify ROS_DISTRO for doc jobs than to implicitly specify it like this?

@cottsay cottsay linked an issue Dec 11, 2024 that may be closed by this pull request
@tfoote
Copy link
Member

tfoote commented Dec 12, 2024

  1. I think that would be entirely appropriate and should be done. I think it's an oversight not a decision.

I'm a little bit split on whether to embed it like this versus put it into the config. The config approach is more visible, but it will cause more need for maintenance of the config files. But we only really update those config files approximately once per distro so that maintenance overhead I think shouldn't be too high. And if we fill the values in now, every future one will copy and paste that as a reference. So I would bias towards enabling build_environment_variables and embedding ROS_DISTRO in via that.

@cottsay
Copy link
Member

cottsay commented Dec 12, 2024

I think that would be entirely appropriate and should be done. I think it's an oversight not a decision.

Thanks, done in #1079.

So I would bias towards enabling build_environment_variables and embedding ROS_DISTRO in via that.

It looks like ROS_PYTHON_VERSION is already set in the build configs (though it wasn't used until now). With that feature working, the buildfarm configs are a lower-risk place to introduce new variables. If we find that solution to be inadequate in the future, we can revisit adding ROS_DISTRO implicitly (as is presented here).

@nuclearsandwich
Copy link
Contributor

Would it be appropriate to plumb build_environment_variables into doc jobs? It'd be simple to do.

I'm also in favor (perhaps obviously since I just reviewed and approved #1079).

@tfoote
Copy link
Member

tfoote commented Dec 13, 2024

I made the follow up to the config to fill in the environment here: ros2/ros_buildfarm_config#314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable doc jobs to know their ROS_DISTRO
4 participants