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

Add support for custom_rosdep_urls in build files #1055

Merged
merged 11 commits into from
Jun 24, 2024

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Jun 6, 2024

In addition to the existing support of the parameter custom_rosdep_urls for the doc and source files, the PR adds the support for the build files so it can be used for things like nightly jobs.

I tested it here https://citest.build.osrfoundation.org/view/Rci/job/Rci__nightly-release_ubuntu_noble_amd64/26/ which builds gz-rendering8 (outside of ROS) using the packages.osrfoundation.org repository and an external rosdep rule to resolve libogre-next-2.3-dev to the package of the same name hosted in packages.o.o.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding documentation.

The block you added the documentation to is specific to CI jobs, but the file you added the actual configuration support to is global to many job types. I think it might be more appropriate to add the config to ci_build_file.py rather than the global build_file.py.

Alternatively, we could keep the config change in build_file.py and move the documentation block, but in that case I think we would need to remove the configuration and documentation in the other specific jobs that already have it, and now inherit support from build_file.py. That said, I'm not sure that every job type that inherits from build_file.py can use rosdep to begin with, so I'm a little wary of this approach.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero
Copy link
Contributor Author

The block you added the documentation to is specific to CI jobs, but the file you added the actual configuration support to is global to many job types. I think it might be more appropriate to add the config to ci_build_file.py rather than the global build_file.py.

Alternatively, we could keep the config change in build_file.py and move the documentation block, but in that case I think we would need to remove the configuration and documentation in the other specific jobs that already have it, and now inherit support from build_file.py. That said, I'm not sure that every job type that inherits from build_file.py can use rosdep to begin with, so I'm a little wary of this approach.

Thanks for the hint and the context. I agree on going conservative and restrict the change to the ci_build_file instead of trying to push a more general approach that we are not sure how it would go in some scenarios.

Changed the code in 7884b1a

@j-rivero j-rivero requested a review from cottsay June 17, 2024 14:59
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@j-rivero j-rivero merged commit 585cdc9 into master Jun 24, 2024
24 checks passed
@j-rivero j-rivero deleted the jrivero/custom_rosdep_ci branch June 24, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants