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

Fix test_config.h to include real engines found (gz-rendering6) #1089

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Dec 3, 2024

🦟 Bug fix

Fixes #

Summary

Instead of hardcoding them by architecture, create the right list of engines from the support found by CMake.

This change will enable ogre1 tests for Windows currently disabled.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Instead of hardcoding them by architecture, create the right
list of engines from the support found by CMake.

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero j-rivero requested a review from iche033 as a code owner December 3, 2024 17:20
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 3, 2024
@j-rivero j-rivero changed the title Fix test_config.h to include real engines found Fix test_config.h to include real engines found (gz-rendering6) Dec 3, 2024
iche033
iche033 previously approved these changes Dec 3, 2024
@iche033 iche033 dismissed their stale review December 3, 2024 18:14

some comments

/// \todo(anyone) re-enable ogre2 test once ogre 2.2 works on macOS
#ifdef __APPLE__
static const std::vector<const char *> kRenderEngineTestValues{"ogre", "optix"};
#else
static const std::vector<const char *> kRenderEngineTestValues{"ogre2", "optix"};
#if defined(HAVE_OGRE) && defined(HAVE_OPTIX) && defined(HAVE_OGRE2)
static const std::vector<const char *> kRenderEngineTestValues{"ogre", "ogre2", "optix"};
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember why we have the weird logic with hardcoded engine names. In fortress, we can't run ogre and ogre2 tests one after another due to conflicting symbols in the dynamic library. So if both ogre and ogre2 exists, we prefer running the test with ogre2. In this case it would be:

static const std::vector<const char *> kRenderEngineTestValues{"ogre2", "optix"};

Note that the tests fixture is revamped in #685 so Harmonic does not have this issue any more.

#elif defined(HAVE_OGRE) && defined(HAVE_OPTIX)
static const std::vector<const char *> kRenderEngineTestValues{"ogre", "optix"};
#elif defined(HAVE_OGRE) && defined(HAVE_OGRE2)
static const std::vector<const char *> kRenderEngineTestValues{"ogre", "ogre2"};
Copy link
Contributor

Choose a reason for hiding this comment

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

see above, we can't have ogre running with ogre2, so this would be:

static const std::vector<const char *> kRenderEngineTestValues{"ogre2"};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iche033
Copy link
Contributor

iche033 commented Dec 3, 2024

I think gz_rendering-ci-pr_any-focal-amd64 is failing because of the reason mentioned in the comments - we can't run ogre and ogre2 tests one after another in the same process.

#include <vector>
#include <gz/common/Util.hh>
#include <gz/rendering/config.hh>

/// \todo(anyone) re-enable ogre2 test once ogre 2.2 works on macOS
#ifdef __APPLE__
static const std::vector<const char *> kRenderEngineTestValues{"ogre", "optix"};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iche033 does it make sense to have optix in Mac?

Copy link
Contributor

Choose a reason for hiding this comment

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

we never tested it on mac so I think can just remove optix here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@j-rivero
Copy link
Contributor Author

j-rivero commented Dec 3, 2024

gz_rendering-pr-winBuild finished. 131 tests run, 0 skipped, 0 failed.

This is failing because the parser log founds Err occurrences in the log. It is working as expected.

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero j-rivero enabled auto-merge (squash) December 4, 2024 18:20
@j-rivero j-rivero disabled auto-merge December 4, 2024 18:20
@j-rivero j-rivero merged commit 46cde99 into ign-rendering6 Dec 4, 2024
9 of 10 checks passed
@j-rivero j-rivero deleted the jrivero/fix_test_config_rendering6 branch December 4, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants