fmauch
commented
6 months ago Thank you for your valuable input there! As you know, I'm currently a bit short on time so please excuse any delay in answering :-)
* I also had to move your reusable ici file from control.ros.org because it uses the TARGET_WORKSPACE instead of UPSTREAM_WORKSPACE (I didn't know how to make the two inputs "one of both required" and how to set a useful default value of TARGET_WORKSPACE) [[CI] Use ros2_control_ci control.ros.org#244](https://github.com/ros-controls/control.ros.org/pull/244)
That second workflow irritated me at first, now I understand why it is there (I completely forgot that I added this...). From my understanding industrial CI always uses a TARGET_WORKSPACE, by default it is simply the respective repo itself. So, it is not necessarily requiring either or, but it's two independent optional choices:
- Do you want to specify an upstream workspace?
- Do you want to use a different target workspace than the current repo?
With that, both use cases could be mapped into the existing ici workflow simply by adding the TARGET_WORKSPACE env variable as an optional one. But that seems to get out of scope for this PR but could be a follow-up once we decided how to progress here.
* Then I thought, maybe we should move your build jobs from control.ros.org to ros2_control_ci? The badge in the sphinx doc would work, too. And failing jobs would be forgotten here and there without anyone subscribing to it ;)
I completely agree with this. It seems much more obvious to actually check the full framework build in a dedicated CI repo (now that we have one) instead of inside the documentation repo. This would go in line with what I've written above with using the "default" ici workflow with an optionally defined TARGET_WS (and potentially DOWNSTREAM_WS)
* Should we then just use the DOWNSTREAM_WORKSPACE in these scheduled builds here? Instead of adding them to every repo and increase build time significantly there? At least I wouldn't add them to the PRs. But if we should add to the repos as scheduled/push triggers or here? Don't know. @bmagyar?
I do not have a clear opinion on this. Having this inside the individual repos has a couple of advantages in my opinion:
- Relevance for downstream packages could be completely different across repos. E.g. some downstream packages might only be relevant for changes in the controller_manager, while others are closely coupled to controllers.
- Having the checks (and potentially failures) inside the repo itself will contribute towards being aware of a problem before actually making a release. If it is inside this common repo, I'm afraid the information might get lost.
Of course this comes with a higher maintenance overhead and increased build times inside the repos, so moving this here might actually be the best tradeoff...
christophfroehlich
This would help raising awareness of breaking things downstream before actually releasing.
In the past we broke things downstream e.g. for ur_robot_driver or most recently webots.
With this change we could add a set of downstream packages that we would like to check when introducing changes.
This could either be incorporated into all builds (in each PR) or into a separate workflow that e.g. runs on pushes to the main (or respective distro) branch. This would show unintentional API breaks and / or integration test failures before actually releasing things to the buildfarm.