Skip to content

Conversation

traversaro
Copy link
Member

@traversaro traversaro commented Apr 4, 2025

As we introduced several modifications in 2025, such as the use of rosdistro snapshot instead of always building the latest rosdistro, and the use of pkg_additional_info.yaml to bump the build number of packages that we want to rebuild, the check for vinca_*.yaml modification before running PR does not make a lot of sense. If a PR is not triggering the build of new packages, then the build step is super fast, and if instead a new recipe is triggered without us being aware of this, it is important that the PR CI catches the unintentional package generation.

@traversaro
Copy link
Member Author

I have no idea why the PR job is not starting, see also #278 .

@traversaro
Copy link
Member Author

I have no idea why the PR job is not starting, see also #278 .

Also on ros-jazzy the PR jobs are not starting: RoboStack/ros-jazzy#40 .

@traversaro
Copy link
Member Author

Also on ros-noetic: RoboStack/ros-noetic#527 .

@traversaro
Copy link
Member Author

In other orgs PR jobs are started normally: robotology/idyntree#1235 .

@traversaro traversaro closed this Apr 4, 2025
@traversaro traversaro reopened this Apr 4, 2025
@traversaro
Copy link
Member Author

In other orgs PR jobs are started normally: robotology/idyntree#1235 .

Also existing PR work fine: #276 .

@traversaro traversaro closed this Apr 4, 2025
@traversaro traversaro reopened this Apr 4, 2025
@traversaro
Copy link
Member Author

Also creating a PR from a fork not from traversaro organization does not work: #279 .

@traversaro
Copy link
Member Author

Also creating a PR from a fork not from traversaro organization does not work: #279 .

The issue was the check that PR started only if .yaml files were modified, and all these PRs only modified .yml files.

@Tobias-Fischer
Copy link
Contributor

Happy to merge once we apply the same fix as in jazzy

@traversaro
Copy link
Member Author

Done in 62ccbc4, let's see if there are some unbuild package for some reason.

@traversaro
Copy link
Member Author

Actually I think we also need to port the RECIPE_CREATED logic from robostack-jazzy, otherwise we will get an error when no recipes are generated.

@traversaro
Copy link
Member Author

Actually I think we also need to port the RECIPE_CREATED logic from robostack-jazzy, otherwise we will get an error when no recipes are generated.

Indeed, Windows is failing for that reason. macOS instead will be fixed by ros-misc-utilities/ffmpeg_encoder_decoder#2 .

@traversaro traversaro changed the title Remove check for .yaml modification before recipe generated in testpr CI job Remove check for .yaml modification before recipe generated in testpr CI job and fix testpr job Apr 8, 2025
@traversaro
Copy link
Member Author

Actually I think we also need to port the RECIPE_CREATED logic from robostack-jazzy, otherwise we will get an error when no recipes are generated.

I am confused, the check seems to be already there.

@traversaro
Copy link
Member Author

Actually I think we also need to port the RECIPE_CREATED logic from robostack-jazzy, otherwise we will get an error when no recipes are generated.

I am confused, the check seems to be already there.

The check was only checking if the recipes directory existed, but that was always happening, so we need also to check if the recipes dir was empty or not. Now the check works as intended, and the PR is ready for review.

@traversaro
Copy link
Member Author

Actually I think we also need to port the RECIPE_CREATED logic from robostack-jazzy, otherwise we will get an error when no recipes are generated.

I am confused, the check seems to be already there.

The check was only checking if the recipes directory existed, but that was always happening, so we need also to check if the recipes dir was empty or not. Now the check works as intended, and the PR is ready for review.

I propagated the fix in RoboStack/ros-jazzy#41 .

@traversaro
Copy link
Member Author

I can also add a fix for #280 in here, but the PR is already quite full of things, so I could do that in a different PR.

@Tobias-Fischer Tobias-Fischer merged commit 88c2889 into RoboStack:main Apr 8, 2025
5 checks passed
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