-
Notifications
You must be signed in to change notification settings - Fork 2k
Updated the osgearth
recipe to support v3.7.2.
#28101
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
base: master
Are you sure you want to change the base?
Updated the osgearth
recipe to support v3.7.2.
#28101
Conversation
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
I am marking this PR as a draft because I have yet to update the |
'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 |
There was a problem hiding this comment.
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
orpatch_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?
There was a problem hiding this comment.
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.
recipes/osgearth/all/conanfile.py
Outdated
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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:
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 |
Time to update the documentation, thanks for pointing this out :) |
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.
I have updated the |
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.
I removed support for |
There was a problem hiding this 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 :)
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. |
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.
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.
@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.
To be clear, I do not expect the |
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.
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.
@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: |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Summary
Changes to recipe: osgearth/3.7.2
Motivation
The official Conan recipe for
osgearth
(located atrecipes\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 oldconans.CMake
type, which are either deprecated or removed in Conan 2.Details
This PR updates the
OsgearthConan
class (that is, the official Conan recipe forosgearth
) 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 theCMAKE_*_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 ofOSG_LIBRARY_POSTFIX
in their file name. This is a preprocessor macro which is set to match the value ofCMAKE_*_POSTFIX
during the build of osgDB; since OpenSceneGraph uses a particular set of values for theCMAKE_*_POSTFIX
variables, we want the osgDB plugins generated byosgearth
to also use these values.For some reason, the
OsgearthConan::requirements()
member function would addzstd
as a dependency when thebuild_zip_plugin
option was set toTrue
. In reality, however, theosgdb_zip
CMake target relies onlibzip
, and notzstd
. 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 theosgdb_osgearth_cache_rocksdb
CMake target. This means that we cannot use a shared build ofrocksdb
in order to build that particular plugin. Thus, theOsgearthConan::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 optionalimgui
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 ofimgui
(andimnodes
) which is used to build theimgui
nodekit, but only if GLEW exists as a dependency.osgearth
v3.3 removes the embedded version ofimgui
(andimnodes
), effectively rendering theimgui
nodekit unusable (that is, unless the user actually copies their own version ofimgui
into the source directory).osgearth
v3.7.2 yet again includes an embedded version ofimgui
(andimnodes
), and yet again, GLEW must exist as a dependency in order to build theimgui
nodekit. This time, however, there is an additional requirement: theOSGEARTH_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 theimgui
nodekit. Future work would be to add optional support for it in the form of a proposedbuild_imgui_nodekit
option.There also appears to be some issues with the
build_procedural_nodekit
option in older versions ofosgearth
. Specifically, theosgEarthProcedural
CMake target does not seem to build on the MSVC for these versions. TheOsgearthConan::config_options()
member function seems to check for this, but doing so there is arguably pointless because any changes made to that option from withinOsgearthConan::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 forosgearth
v3.7.2; theosgEarthProcedural
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:
openscenegraph
build failures. #28121osgearth
andgdal
. #28126