Skip to content

Conversation

tylerbrawl
Copy link
Contributor

@tylerbrawl tylerbrawl commented Aug 5, 2025

Summary

Changes to recipe: osgearth/3.7.2

Motivation

The official Conan recipe for osgearth (located at recipes\osgearth\all\conanfile.py) appears to have been neglected for a long time. Consider, for example, the following:

  • There existed several dependency conflicts based on the versioned references specified in the osgearth recipe.

  • OpenSceneGraph and GDAL were not building on the latest version of the MSVC. (These are both mandatory dependencies of osgearth.)

  • Several of the member functions of OsgearthConan made use of old Conan 1 facilities, such as the old conans.CMake type, which are either deprecated or removed in Conan 2.

Details

This PR updates the OsgearthConan class (that is, the official Conan recipe for osgearth) to support Conan 2 in addition to a new version of the library - v3.7.2. (Support for both v3.2 and v3.3 has been retained in this commit. However, this is subject to change pending a code review.) This required a significant number of changes to be made to its implementation. Along the way, several bugs in the previous implementation were encountered and fixed. These are as follows:

  • The OsgearthConan::generate() member function will now set the CMAKE_*_POSTFIX CMake variables in the CMake cache in order to match what is expected by osgDB. Specifically, any plugins which are loaded by osgDB in a shared/DLL build of OpenSceneGraph are expected to have a postfix of OSG_LIBRARY_POSTFIX in their file name. This is a preprocessor macro which is set to match the value of CMAKE_*_POSTFIX during the build of osgDB; since OpenSceneGraph uses a particular set of values for the CMAKE_*_POSTFIX variables, we want the osgDB plugins generated by osgearth to also use these values.

  • For some reason, the OsgearthConan::requirements() member function would add zstd as a dependency when the build_zip_plugin option was set to True. In reality, however, the osgdb_zip CMake target relies on libzip, and not zstd. This dependency has thus been adjusted accordingly.

  • The official Conan recipe for rocksdb will delete the C++ headers from the packaged include directory. (A comment claims that this is done for the sake of ABI stability.) However, osgearth relies on these headers to exist in order to build the osgdb_osgearth_cache_rocksdb CMake target. This means that we cannot use a shared build of rocksdb in order to build that particular plugin. Thus, the OsgearthConan::validate() member function has been updated to check for this particular case.

There are some additional caveats which I would like to mention here. osgearth includes an optional imgui integration/nodekit which has historically been very problematic. Consider, for example, the confusing evolution of how this integration was distributed:

  • osgearth v3.2 includes an embedded version of imgui (and imnodes) which is used to build the imgui nodekit, but only if GLEW exists as a dependency.

  • osgearth v3.3 removes the embedded version of imgui (and imnodes), effectively rendering the imgui nodekit unusable (that is, unless the user actually copies their own version of imgui into the source directory).

  • osgearth v3.7.2 yet again includes an embedded version of imgui (and imnodes), and yet again, GLEW must exist as a dependency in order to build the imgui nodekit. This time, however, there is an additional requirement: the OSGEARTH_BUILD_IMGUI_NODEKIT CMake variable must also be set.

It should be possible to patch osgearth to ensure consistency between all of our supported versions. However, this is expected to take a significant amount of effort. This PR thus always disables the imgui nodekit. Future work would be to add optional support for it in the form of a proposed build_imgui_nodekit option.

There also appears to be some issues with the build_procedural_nodekit option in older versions of osgearth. Specifically, the osgEarthProcedural CMake target does not seem to build on the MSVC for these versions. The OsgearthConan::config_options() member function seems to check for this, but doing so there is arguably pointless because any changes made to that option from within OsgearthConan::config_options() will be overwritten by the value specified in, e.g., the current Conan profile. No explanation is given as to why this check is done, but it seems incorrect. (It should be noted that this does not appear to be an issue for osgearth v3.7.2; the osgEarthProcedural CMake target builds fine on my machine with the MSVC in that version.)

fixes #25927

Blocking PRs

The following PRs contain changes which must be merged before the changes in this PR can have any meaningful impact:


The official Conan recipe for `osgearth` (located at
`recipes\osgearth\all\conanfile.py`) appears to have been neglected
for a long time. Consider, for example, the following:

  * There existed several dependency conflicts based on the
    versioned references specified in the `osgearth` recipe.

  * OpenSceneGraph and GDAL were not building on the latest version
    of the MSVC. (These are both mandatory dependencies of
    `osgearth`.)

  * Several of the member functions of `OsgearthConan` made use of
    old Conan 1 facilities, such as the old `conans.CMake` type,
    which are either deprecated or removed in Conan 2.

This commit updates the `OsgearthConan` class (that is, the
official Conan recipe for `osgearth`) to support Conan 2 in
addition to a new version of the library - v3.7.2. (Support for
both v3.2 and v3.3 has been retained in this commit. However, this
is subject to change pending a code review.) This required a
significant number of changes to be made to its implementation.
Along the way, several bugs in the previous implementation were
encountered and fixed. These are as follows:

  * The `OsgearthConan::generate()` member function will now set
    the `CMAKE_*_POSTFIX` CMake variables in the CMake cache in
    order to match what is expected by osgDB. Specifically, any
    plugins which are loaded by osgDB in a shared/DLL build of
    OpenSceneGraph are expected to have a postfix of
    `OSG_LIBRARY_POSTFIX` in their file name. This is a
    preprocessor macro which is set to match the value of
    `CMAKE_*_POSTFIX` during the build of osgDB; since
    OpenSceneGraph uses a particular set of values for the
    `CMAKE_*_POSTFIX` variables, we want the osgDB plugins
    generated by `osgearth` to also use these values.

  * For some reason, the `OsgearthConan::requirements()` member
    function would add `zstd` as a dependency when the
    `build_zip_plugin` option was set to `True`. In reality,
    however, the `osgdb_zip` CMake target relies on `libzip`, and
    not `zstd`. This dependency has thus been adjusted accordingly
    in this commit.

  * The official Conan recipe for `rocksdb` will delete the C++
    headers from the packaged include directory. (A comment claims
    that this is done for the sake of ABI stability.) However,
    `osgearth` relies on these headers to exist in order to build
    the `osgdb_osgearth_cache_rocksdb` CMake target. This means
    that we cannot use a shared build of `rocksdb` in order to
    build that particular plugin. Thus, this commit updates the
    `OsgearthConan::validate()` member function to check for this
    particular case.

There are some additional caveats which I would like to mention
here. `osgearth` includes an optional `imgui` integration/nodekit
which has historically been very problematic. Consider, for
example, the confusing evolution of how this integration was
distributed:

  * `osgearth` v3.2 includes an embedded version of `imgui` (and
    `imnodes`) which is used to build the `imgui` nodekit, but only
    if GLEW exists as a dependency.

  * `osgearth` v3.3 removes the embedded version of `imgui` (and
    `imnodes`), effectively rendering the `imgui` nodekit unusable
    (that is, unless the user actually copies their own version of
    `imgui` into the source directory).

  * `osgearth` v3.7.2 yet again includes an embedded version of
    `imgui` (and `imnodes`), and yet again, GLEW must exist as a
    dependency in order to build the `imgui` nodekit. This time,
    however, there is an additional requirement: the
    `OSGEARTH_BUILD_IMGUI_NODEKIT` CMake variable must also be set.

It should be possible to patch `osgearth` to ensure consistency
between all of our supported versions. However, this is expected to
take a significant amount of effort. This commit thus always
disables the `imgui` nodekit. Future work would be to add optional
support for it in the form of a proposed `build_imgui_nodekit`
option.

There also appears to be some issues with the
`build_procedural_nodekit` option in older versions of `osgearth`.
Specifically, the `osgEarthProcedural` CMake target does not seem
to build on the MSVC for these versions. The
`OsgearthConan::config_options()` member function seems to check
for this, but doing so there is arguably pointless because any
changes made to that option from within
`OsgearthConan::config_options()` will be overwritten by the value
specified in, e.g., the current Conan profile. No explanation is
given as to why this check is done, but it seems incorrect. (It
should be noted that this does not appear to be an issue for
`osgearth` v3.7.2; the `osgEarthProcedural` CMake target builds
fine on my machine with the MSVC in that version.)

Fixes: conan-io#25927
@tylerbrawl
Copy link
Contributor Author

I am marking this PR as a draft because I have yet to update the recipes\osgearth\all\test_package\conanfile.py testing recipe. However, I have tested the new osgearth recipe locally on my own machine under several different configurations.

'3.3':
sha256: 4b4a8ba70e707c6aae7d2fe2904b8761e9827398ddeb60633938fe486f5fa622
url: https://github.com/gwaldron/osgearth/archive/refs/tags/osgearth-3.3.tar.gz
'3.2':
sha256: 7e1dd643b1f3b8d1ba9561b899c18176af988342b86c42d89a70be924cb747f6
url: https://github.com/gwaldron/osgearth/archive/refs/tags/osgearth-3.2.tar.gz
patches:
'3.7.2':
- patch_file: patches/3.7.2-osgearth.patch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conandata.yml documentation would indicate that there are numerous problems with this new patch:

  • There is no patch_description for this patch.
  • There are no patch_type or patch_source fields for this patch. (NOTE: These are only listed as recommended requirements.)

I left this patch undocumented because it was created by migrating the changes from the other patches to v3.7.2, and said patches were themselves undocumented. Thus, I couldn't really tell you why (or even if, honestly) this patch needs to exist without further investigation.

That begs the question, though: Why aren't the other patches documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous patch files were removed in commit 741efa2. However, the 3.7.2-osgearth.patch file remains undocumented for the reasons given above.

tools.check_min_cppstd(self, 11)
elif self.settings.compiler == "apple-clang":
raise ConanInvalidConfiguration("With apple-clang cppstd needs to be set, since default is not at least c++11.")
check_min_cppstd(self, self._minimum_cpp_standard)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it is ultimately pointless to allow the user to set the C++ Standard for osgearth. This is because the root CMakeLists.txt file for that project will set the CMAKE_CXX_STANDARD CMake variable to a hard-coded constant based on its minimum required C++ Standard.

Should we be patching that file to respect the user's choice of C++ Standard?

Copy link
Member

Choose a reason for hiding this comment

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

We usually remove such set(CMAKE_CXX_STANDARD ... calls, to let user profiles choose the final version, but lately we're looking into just keeping whatever upstream does, but for a final answer on this, I'll ping @jcar87

@AbrilRBS
Copy link
Member

AbrilRBS commented Aug 5, 2025

Hi @tylerbrawl thanks a lot for taking the time create a PR for this, we truly appreciate it!

Just skimmed the PR for now, but:

Support for both v3.2 and v3.3 has been retained in this commit. However, this is subject to change pending a code review.

If this simplifies the recipe, we'd be happy to drop support for the older versions, and keep only the latest one, thanks!

@tylerbrawl
Copy link
Contributor Author

If this simplifies the recipe, we'd be happy to drop support for the older versions, and keep only the latest one, thanks!

That would certainly simplify the recipe, but the Sources and Patches documentation states that we should include support for at least two versions of the "latest major release." (I assume that would be osgearth v3.x.y in this case.) Would I also need to add support for osgearth v3.7.1?

@jcar87
Copy link
Contributor

jcar87 commented Aug 6, 2025

If this simplifies the recipe, we'd be happy to drop support for the older versions, and keep only the latest one, thanks!

That would certainly simplify the recipe, but the Sources and Patches documentation states that we should include support for at least two versions of the "latest major release." (I assume that would be osgearth v3.x.y in this case.) Would I also need to add support for osgearth v3.7.1?

Time to update the documentation, thanks for pointing this out :)
In general we ask that PRs start off with the minimal set of changes required to reach the objective, everything else we can take care of.

This commit updates the testing recipe for `osgearth`. This recipe
is located at `recipes\osgearth\all\test_package\conanfile.py`.
The root `CMakeLists.txt` for this testing recipe has been updated
to resemble the examples given in
[the Conan 2 documentation](https://docs.conan.io/2/tutorial/creating_packages/test_conan_packages.html),
as has the `TestPackageConan` class defined in
`recipes\osgearth\all\test_package\conanfile.py`.

During this testing, it was revealed that the `test_package` CMake
target managed by the testing recipe failed to build. This was
ultimately because Conan does not make the headers of dependencies
transitive by default; this must be explicitly requested when
calling the `ConanFile::requires()` member function by setting the
`transitive_headers` parameter to `True`. Since the public headers
of `osgearth` will frequently attempt to `#include` the public
headers of `openscenegraph`, it makes sense to use transitive
headers for that particular dependency. This conclusion resulted in
the `OsgearthConan::requirements()` member function being updated
accordingly in this commit.
@tylerbrawl
Copy link
Contributor Author

I have updated the test_package recipe for osgearth in commit 5ef3894. I will mark this PR as ready for review.

@tylerbrawl tylerbrawl marked this pull request as ready for review August 6, 2025 15:59
This commit removes support for the following versions of
`osgearth` from its official Conan recipe:

  * v3.2
  * v3.3

This change came from a suggestion by @AbrilRBS that the older
versions could be removed if doing so would simplify the final
`osgearth` recipe. As can be seen from the changes in this commit,
this did, in fact, result in a much simpler implementation for the
`OsgearthConan` class. The `CMakeLists.txt` file which was
previously being injected into the `osgearth` build (via
`OsgearthConan.exports_source`) has been removed in this commit, as
the CMake scripts used in `osgearth` v3.7.2 make proper use of the
`find_package()` CMake command (in most cases, anyways).

This commit also deprecates the following options, as described in
[the FAQs documentation](https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#can-i-remove-an-option-from-a-recipe):

  * `build_leveldb_cache`
  * `with_draco`

These options are ultimately useless in `osgearth` v3.7.2. In
previous versions, they were used to set the values of various
CMake variables, but said variables no longer exist.
@tylerbrawl
Copy link
Contributor Author

I removed support for osgearth v3.2 and v3.3 in commit 741efa2. The resulting recipe is a lot simpler, in my opinion, but I'll defer to the maintainers to make the final decision here.

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Some comments on the osgearth recipe itself, which looks great so far, thanks a lot for taking the time to contribute it, we appreciate it

As for gdal and openscenegraph, different PRs for each would ease their review by quite a lot :)

@tylerbrawl
Copy link
Contributor Author

As for gdal and openscenegraph, different PRs for each would ease their review by quite a lot :)

Sure! For OSG in particular, one of the patches already exists in a separate open PR as #26231. I could create a new PR containing both the duplicate patch and the new patch if we want to just close that one. Otherwise, I can wait for that one to be handled.

tylerbrawl and others added 4 commits August 6, 2025 14:03
Co-authored-by: Abril Rincón Blanco <5364255+AbrilRBS@users.noreply.github.com>
Apparently, the source files can be patched directly within
`ConanFile::source()`. Thanks to @AbrilRBS for the suggestion!
…recipe.

This commit simplifies the `osgearth` recipe by removing a number
of options which were ultimately left unused. It also removes
several "hypothetical options" - that is, comments which usually
take the form of code declaring or accessing an option. The
removals can be categorized into one of several groups.

First, the following deprecated options were removed in this
commit:

  * `build_leveldb_cache`
  * `with_draco`

These options have no use whatsoever in `osgearth` v3.7.2. They
were first converted into deprecated options based on the general
guidance provided in
[the FAQs](https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#can-i-remove-an-option-from-a-recipe).
During a code review, however, @AbrilRBS suggested that they could
simply be removed because the `osgearth` recipe had not been
updated to support Conan 2 prior to this commit.

Second, the following so-called "hypothetical options" were
removed, as suggested during a code review:

  * `build_imgui_nodekit`
  * `build_silverlining_nodekit`
  * `build_triton_nodekit`
  * `with_basisu`

Finally, several additional options were removed in this commit for
ultimately being pointless:

  * `enable_nvtt_cpu_mipmaps`: In `osgearth` v3.3, this option
    would be used to set the `OSGEARTH_ENABLE_NVTT_CPU_MIPMAPS`
    CMake variable. However, this variable is no longer used in
    `osgearth` v3.7.2.

  * `with_sqlite3`: In `osgearth` v3.7.2, `sqlite3` is a mandatory
    dependency. Thus, it does not make much sense to retain this
    option.
The official Conan recipe for `osgearth` would historically install
shaders into a directory called `res`, located at the root of the
package folder. During a code review, however, @AbrilRBS asked that
the shader installation directory used by `osgearth` be retained,
rather than renamed to match the behavior of previous versions of
the `osgearth` recipe. This is because the old recipe had not been
updated to Conan 2 prior to conan-io#28101.

Based on that feedback, this commit updates the
`OsgearthConan::package()` member function to no longer attempt to
rename the shader installation directory. For all intents and
purposes, future recipe versions should consider the `share` folder
as the destination for installed shaders.
tylerbrawl added a commit to tylerbrawl/conan-center-index that referenced this pull request Aug 7, 2025
This commit includes several additional patch files in order to
resolve build failures which occurred when attempting to build
`openscenegraph` v3.6.5 with the MSVC. An adjustment was also made
to the declared required `libjpeg` version; this was done to
resolve a dependency conflict for conan-io#28101.

I should mention that I have not done any significant analysis
regarding the implications of increasing the `libjpeg` version
requirement. This change resolves the aforementioned dependency
conflict, but it could also theoretically introduce new ones. It
may be better to replace this with a version range, instead.
@AbrilRBS
Copy link
Member

AbrilRBS commented Aug 7, 2025

@tylerbrawl ping me once you remove gdal and openscenegraph from this PR, and I'll trigger the CI to build this PR :)

This commit removes the changes to the `openscenegraph` recipe
which were made pursuant to conan-io#28101. Instead, these changes are now
contained within PR conan-io#28121.
@tylerbrawl
Copy link
Contributor Author

@tylerbrawl ping me once you remove gdal and openscenegraph from this PR, and I'll trigger the CI to build this PR :)

To be clear, I do not expect the osgearth recipe to build successfully without the changes for gdal and openscenegraph. I will let you know when those PRs are created, though.

tylerbrawl added a commit to tylerbrawl/conan-center-index that referenced this pull request Aug 7, 2025
This commit adjusts the following dependencies declared in the
`GdalConan::requirements()` member function:

  * `libjpeg`
  * `sqlite3`

These now use an explicit version range, rather than an exact
version specification, in order to resolve conflicts with the
`osgearth` recipe as updated in conan-io#28101.

In addition, this commit also adds a new member function called
`GdalConan::validate_build()` which throws a
`ConanInvalidConfiguration` exception if the user attempts to build
GDAL with the MSVC using a C++ Standard which is greater than
C++17. This was done to account for build failures which were being
encountered in that particular case.

(NOTE: All of the changes discussed in this commit message apply to
the `GdalConan` class defined within
`recipes\gdal\pre_3.5.0\conanfile.py`. For versions of GDAL which
are at least as high as v3.5.0, a different recipe is used,
instead. This commit does not attempt to update that recipe.)
This commit removes the changes to the `gdal` recipe which were
made pursuant to conan-io#28101. Instead, these changes are now contained
within PR conan-io#28126.
@tylerbrawl
Copy link
Contributor Author

@AbrilRBS - I have created #28121 and #28126 for openscenegraph and gdal, respectively. Only the osgearth recipe has been modified in this PR.

@AbrilRBS
Copy link
Member

AbrilRBS commented Aug 8, 2025

Great! Meanwhile, the CI will now try to run after every push now, and the linter has detected some issues that youj might want to tackle while the other PRs get merged.

Once that looks good, we can override on our side the conflicting versions and see if the recipe builds (But for the final merge we will need the other PRs to get merged beforehand)

This commit addresses several issues which the Recipe Linter
diagnosed for the `osgearth` recipe. These mostly consist of unused
`import` statements, but it did warn about a missing `package_type`
attribute for `OsgearthConan`. (There was also an "error" for not
using `conan.tools.files.export_conandata_patches()`, although I
suspect that the previous use of `OsgearthConan.exports_sources`
achieved equivalent results.)

This commit also updates the `required_conan_version` value for the
`osgearth` recipe to `">=2.0"` based on code review feedback. It
still isn't quite clear if this is correct, though, because I have
not actually tested the recipe at that version.
@tylerbrawl
Copy link
Contributor Author

@AbrilRBS - It looks like the CI isn't running to account for the new commit. I primarily want to verify that the Recipe Linter issues are resolved, if possible.

get(self, **self.conan_data["sources"][self.version], strip_root=True)
apply_conandata_patches(self)

def _get_library_postfix(self, build_type: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Hi @tylerbrawl - coming back to this, the only remaining doubt about this recipe for me now is this function.

Could you share some more info about this? Correct me if I'm missing something, but the idea is that the dependency openscenegraph expects to load plugins with a given postfix when using it, with no way to override the behaviour, and that we then need to create our libraries with said postfixes?

I wanted to know more because we usually try to refrain from modifying the library names that recipes generate unless absolutely necessary, so I'm wondering if there could be simpler/different approaches :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your interpretation is correct. However, what I don't understand is why we need to do this ourselves. Intuitively, I would have expected the CMake scripts used by osgearth to already create the plugins with the appropriate postfix. Yet, that doesn't appear to be what is happening.

It might be worth submitting an issue for this on the GitHub page for osgearth. That would solve the problem for newer versions of the library, but not for osgearth v3.7.2.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think raising this with upstream would be worth it if only to let them know.

Do note that other package managers that have this library seem not to care about the output name of the plugins, so I wonder then how they are making them work, if at all

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.

[request] <osgearth>/<3.3>
3 participants