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 more explicit version compatibility for OGRE2 #297

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

mjcarroll
Copy link
Contributor

🦟 Bug fix

Fixes #295

Summary

Before this patch, it was possible for a gz_find_package() to return an incompatible version of OGRE2. This patch adjusts the logic to insure that exact (or compatible) matches are surfaced.

I have added a simple test to verify the behavior here: https://github.com/mjcarroll/ogre23_docker_test

It can be tested via building/running a docker container, for example

git clone https://github.com/mjcarroll/ogre23_docker_test
make jammy-2.2  # Build gz-cmake on jammy with only ogre 2.2 installed
make jammy-2.3  # Build gz-cmake on jammy with only ogre 2.3 installed
make jammy-both  # Build gz-cmake on jammy with both ogre 2.2 and ogre 2.3 installed

The expected output for the "both" case is that:

  • the package requesting 2.1 gets NOT_FOUND as there is no matching compatible version.
  • the package requesting 2.2 gets version 2.2.5
  • the package requesting 2.3 gets 2.3.1
Starting >>> ogre-2.1
[1.052s] WARNING:colcon.colcon_cmake.task.cmake.build:Could not run installation step for package 'ogre-2.1' because it has no 'install' target
--- output: ogre-2.1
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Finding OGRE 2.1
-- Looking for OGRE using the name: OGRE2
--   ! OGRE2 not found
-- Looking for OGRE using the name: OGRE-Next
--   ! OGRE-Next found with incompatible version 2.2.5
-- Looking for GzOGRE2 - not found

-- OGRE2_FOUND: FALSE
-- Configuring done
-- Generating done
-- Build files have been written to: /workspace/build/ogre-2.1
---
Finished <<< ogre-2.1 [0.37s]

Summary: 1 package finished [0.71s]
Starting >>> ogre-2.2
[1.040s] WARNING:colcon.colcon_cmake.task.cmake.build:Could not run installation step for package 'ogre-2.2' because it has no 'install' target
--- output: ogre-2.2
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Finding OGRE 2.2.0
-- Looking for OGRE using the name: OGRE2
--   ! OGRE2 not found
-- Looking for OGRE using the name: OGRE-Next
--   + component HlmsPbs: found
--   + component HlmsUnlit: found
--   + component Overlay: found
-- Looking for GzOGRE2 - found

-- OGRE2_FOUND: TRUE
-- OGRE2_LIBRARIES: /usr/lib/x86_64-linux-gnu/libOgreNextMain.soGzOGRE2-HlmsPbs::GzOGRE2-HlmsPbsGzOGRE2-HlmsUnlit::GzOGRE2-HlmsUnlitGzOGRE2-Overlay::GzOGRE2-Overlay
-- OGRE2_INCLUDE_DIRS: /usr/include/OGRE-Next/usr/include/OGRE-Next/RenderSystems/GL3Plus
-- OGRE2_VERSION: 2.2.5
-- OGRE2_VERSION_MAJOR: 2
-- OGRE2_VERSION_MINOR: 2
-- OGRE2_VERSION_PATCH: 5
-- OGRE2_RESOURCE_PATH: /usr/lib/x86_64-linux-gnu/OGRE-Next
-- GzOGRE2_VERSION_EXACT: FALSE
-- GzOGRE2_VERSION_COMPATIBLE: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: /workspace/build/ogre-2.2
---
Finished <<< ogre-2.2 [0.36s]

Summary: 1 package finished [0.70s]
Starting >>> ogre-2.3
[1.026s] WARNING:colcon.colcon_cmake.task.cmake.build:Could not run installation step for package 'ogre-2.3' because it has no 'install' target
--- output: ogre-2.3
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Finding OGRE 2.3.0
-- Looking for OGRE using the name: OGRE2
--   + component HlmsPbs: found
--   + component HlmsUnlit: found
--   + component Overlay: found
-- Looking for GzOGRE2 - found

-- OGRE2_FOUND: TRUE
-- OGRE2_LIBRARIES: /usr/lib/x86_64-linux-gnu/OGRE-2.3/libOgreNextMain.soGzOGRE2-HlmsPbs::GzOGRE2-HlmsPbsGzOGRE2-HlmsUnlit::GzOGRE2-HlmsUnlitGzOGRE2-Overlay::GzOGRE2-Overlay
-- OGRE2_INCLUDE_DIRS: /usr/include/OGRE-2.3/usr/include/OGRE-2.3/RenderSystems/GL3Plus
-- OGRE2_VERSION: 2.3.1
-- OGRE2_VERSION_MAJOR: 2
-- OGRE2_VERSION_MINOR: 3
-- OGRE2_VERSION_PATCH: 1
-- OGRE2_RESOURCE_PATH: /usr/lib/x86_64-linux-gnu/OGRE-2.3/OGRE
-- GzOGRE2_VERSION_EXACT: FALSE
-- GzOGRE2_VERSION_COMPATIBLE: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: /workspace/build/ogre-2.3
---
Finished <<< ogre-2.3 [0.35s]

Summary: 1 package finished [0.70s]

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.

@mjcarroll mjcarroll self-assigned this Aug 2, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 2, 2022
@mjcarroll
Copy link
Contributor Author

Note that some portion of this will need to be backported to focal and gz-cmake2 so that fortress isn't disrupted by having Ogre 2.3 installed. I will do that once this lands as the logic will be slightly different (fortress doesn't need 2.3)

@chapulina chapulina changed the base branch from main to gz-cmake3 August 3, 2022 01:46
@chapulina chapulina added the bug Something isn't working label Aug 3, 2022
@scpeters
Copy link
Member

scpeters commented Aug 3, 2022

The expected output for the "both" case is that:

  • the package requesting 2.1 gets NOT_FOUND as there is no matching compatible version.
  • the package requesting 2.2 gets version 2.2.5
  • the package requesting 2.3 gets 2.3.1

So translating to the COMPATIBILITY keywords that could be supplied to write_basic_package_version_file (<AnyNewerVersion|SameMajorVersion|SameMinorVersion|ExactVersion>), it sounds like this find module will follow SameMinorVersion?

Also, I'd like to confirm what happens if only a major version 2 is passed as the version string, or if no version string is passed at all.

@mjcarroll
Copy link
Contributor Author

SameMinorVersion

Correct, that is the semantics I was trying to follow. We are using pkg-config to actually search rather than FindOGRE (I think for multiple reasons), so we can't just use the generated package_version_file in this case. (This is at least my understanding, but CMake and CPack are complicated, so there may be a way to make it happen).

Also, I'd like to confirm what happens if only a major version 2 is passed as the version string, or if no version string is passed at all.

I will add test cases for this. I know that any major version outside of 2 will cause it not to be found, to prevent finding OGRE 13 (which will be once again interesting when ogre-next-3.0 comes out).

@mjcarroll
Copy link
Contributor Author

I added a test case for just the major version:

The results in the docker container are "OGRE Not found", which is the desired behavior in this case.

--- output: gz-ogre-2
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Finding OGRE 2
-- Looking for OGRE using the name: OGRE2
--   ! OGRE2 not found
-- Looking for OGRE using the name: OGRE-Next
--   ! OGRE-Next not found
-- Looking for GzOGRE2 - not found

-- OGRE2_FOUND: 
-- Configuring done
-- Generating done
-- Build files have been written to: /workspace/build/gz-ogre-2

@scpeters
Copy link
Member

scpeters commented Aug 5, 2022

I added a test case for just the major version:

The results in the docker container are "OGRE Not found", which is the desired behavior in this case.

I assume it will also give OGRE Not found if no version string is supplied. I would have expected it to be happy with any version of ogre 2 that it finds if the user does not specify a specific version, but if we want to require a version string that includes a minor number, should we give a warning message if the user doesn't supply such a version string?

@mjcarroll mjcarroll force-pushed the mjcarroll/test_exact branch from 91e06c9 to 14a4e7b Compare August 8, 2022 13:33
@mjcarroll mjcarroll force-pushed the mjcarroll/test_exact branch from 14a4e7b to d910a25 Compare August 8, 2022 13:34
@mjcarroll
Copy link
Contributor Author

In this case, there is no real ABI/API compatibility guarantees between minor version, so I believe we need to have users explicitly request a minor version.

I have updated the warnings accordingly.

--- output: gz-ogre
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Finding OGRE 2
CMake Warning at /workspace/install/gz-cmake3/share/cmake/gz-cmake3/cmake3/FindGzOGRE2.cmake:60 (message):
  find_package(GzOGRE2) must be called with a VERSION argument with a minimum
  of major and minor version
Call Stack (most recent call first):
  /workspace/install/gz-cmake3/share/cmake/gz-cmake3/cmake3/GzUtils.cmake:231 (find_package)
  CMakeLists.txt:7 (gz_find_package)


-- Looking for GzOGRE2 - not found

-- OGRE2_FOUND: false
-- Configuring done
-- Generating done
-- Build files have been written to: /workspace/build/gz-ogre
---
--- stderr: gz-ogre
CMake Warning at /workspace/install/gz-cmake3/share/cmake/gz-cmake3/cmake3/FindGzOGRE2.cmake:60 (message):
  find_package(GzOGRE2) must be called with a VERSION argument with a minimum
  of major and minor version
Call Stack (most recent call first):
  /workspace/install/gz-cmake3/share/cmake/gz-cmake3/cmake3/GzUtils.cmake:231 (find_package)
  CMakeLists.txt:7 (gz_find_package)


---
Finished <<< gz-ogre [0.37s]

@mjcarroll mjcarroll merged commit a5c59af into gz-cmake3 Aug 10, 2022
@mjcarroll mjcarroll deleted the mjcarroll/test_exact branch August 10, 2022 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants