Skip to content

Remove check for .yaml modification before recipe generated in testpr CI job and fix testpr job #40

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

Merged
merged 4 commits into from
Apr 8, 2025

Conversation

traversaro
Copy link
Member

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.

xref: RoboStack/ros-humble#277

@Tobias-Fischer
Copy link
Contributor

@traversaro - do you want to remove all checks for any changes in yml/yaml files? Check my latest commit for hints

@traversaro
Copy link
Member Author

@traversaro - do you want to remove all checks for any changes in yml/yaml files? Check my latest commit for hints

Thanks I had totally missed that. Yes, perhaps just running the CI on all PRs seems easier?

All those checks come from pre-snapshot times, but now with snapshot if no new packages build are triggered, the CI check should be a really fast check (modulo bugs), so I think it would be great to just remove the checks and always run CI?

@traversaro
Copy link
Member Author

To clarify, my proposal is fee2a0e . I also added a workflow_dispatch trigger to be able to easily debug the job on PR-less branches if necessary.

@traversaro
Copy link
Member Author

Windows is failing with:

2025-04-07T06:59:49.7001155Z  │ │   CMake Error at cmake_install.cmake:264 (execute_process):
2025-04-07T06:59:49.7003084Z  │ │     Syntax error in cmake code at
2025-04-07T06:59:49.7003784Z  │ │   
2025-04-07T06:59:49.7004199Z  │ │       %SRC_DIR%/build/cmake_install.cmake:266
2025-04-07T06:59:49.7004788Z  │ │   
2025-04-07T06:59:49.7005577Z  │ │     when parsing string
2025-04-07T06:59:49.7005952Z  │ │   
2025-04-07T06:59:49.7006272Z  │ │       %PREFIX%\python.exe
2025-04-07T06:59:49.7006625Z  │ │   
2025-04-07T06:59:49.7006979Z  │ │     Invalid character escape '\b'.

I vaguely remember seeing this as a warning before, so I guess it could make sense to just pin cmake to 3.* for now.

@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 7, 2025
@traversaro traversaro requested review from Tobias-Fischer and wolfv and removed request for Tobias-Fischer April 7, 2025 08:49
@Tobias-Fischer
Copy link
Contributor

So I think we need to read in the snapshot file, otherwise we'll rebuild newer versions of packages that have been built since the last rebuild, and that might lead to ABI incompatibilities.

@traversaro
Copy link
Member Author

So I think we need to read in the snapshot file, otherwise we'll rebuild newer versions of packages that have been built since the last rebuild, and that might lead to ABI incompatibilities

I am probably missing something, but aren't we already reading the snapshot file?

@Tobias-Fischer
Copy link
Contributor

I don’t think we do - it’s neither in the testpr.yml nor main.yml

@traversaro
Copy link
Member Author

I don’t think we do - it’s neither in the testpr.yml nor main.yml

We moved it in the vinca_*.file in #16 , see

patch_dir: patch
rosdistro_snapshot: rosdistro_snapshot.yaml
.

@Tobias-Fischer
Copy link
Contributor

Ah, yes - thanks , jet lagged me!

@Tobias-Fischer Tobias-Fischer merged commit 40eccf4 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