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 test_require_gpu to ci jobs #1059

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add test_require_gpu to ci jobs #1059

wants to merge 4 commits into from

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Jul 2, 2024

Thew PR implements the test_require_gpu parameter for the CI kind of jobs in the buildfarm.

The first commit 4e0466e adds the necessary code to support the test_require_gpu parameter in the ci build files and the docker run necessary arguments to the ci_job template when running "build and test".

The second commit d6a39c3 fixes current code existing in devel jobs used by the ci jobs.

Tested: the code was used for testing gz-rendering OpenGL tests in the testing buildfarm of the Gazebo team. https://citest.build.osrfoundation.org/view/Rci/job/Rci__nightly-release_ubuntu_noble_amd64/68/testReport/projectroot.test/integration/

P.D: there is a need to audit the existing gpu code in the devel jobs that I plan to do in a different PR.

@cottsay
Copy link
Member

cottsay commented Jul 26, 2024

It looks like this rolls back some of #624, which was merged over 4 years ago. Should we tick-tock some of the public-facing changes? In particular, the script argument changes would cause a regression if someone was already using them.

@nuclearsandwich
Copy link
Contributor

It looks like this rolls back some of #624, which was merged over 4 years ago. Should we tick-tock some of the public-facing changes? In particular, the script argument changes would cause a regression if someone was already using them.

I agree. It's profoundly unlikely that we have users on the previous setup but even if they never got it working I think emitting deprecation warnings rather than rendering configs unparseable is good practice.

@nuclearsandwich
Copy link
Contributor

Although we don't actually do releases of ros_buildfarm too frequently ATM so I'm not sure what the criteria for tocking out would be.

@j-rivero
Copy link
Contributor Author

It looks like this rolls back some of #624, which was merged over 4 years ago. Should we tick-tock some of the public-facing changes? In particular, the script argument changes would cause a regression if someone was already using them.

I agree. It's profoundly unlikely that we have users on the previous setup but even if they never got it working I think emitting deprecation warnings rather than rendering configs unparseable is good practice.

Understood. Parameters are back in b367013 with a deprecation warning.

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.

3 participants