OGRECave / ogre-next

aka ogre v2 - scene-oriented, flexible 3D C++ engine
https://ogrecave.github.io/ogre-next/api/latest
Other
1.02k stars 224 forks source link

Renaming libraries in OGRE-Next: libOgre -> libOgreNext #259

Open j-rivero opened 2 years ago

j-rivero commented 2 years ago

Following proposal and conversation in #232, one of the points for being able to play nicely with OGRE 1.x installations would be the name of the libraries. There is a potential risk for a project that deal with a co-installation of both libraries to have the same name for libraries of totally independent projects, i.e: linking against one of the project while at run time the other project libraries are loaded.

The usual way of dealing with this problem in forked projects is to change the name of the project and accordingly the name of the compilation artifacts and the filesystem layout.

A different approach would be to place libraries in a different directory when installing, (in Linux would be to place them under /usr/lib/{ARCH}/Ogre-Next or similar). While that could work, the risk of loading and mixing different libraries with the OGRE-1.x project is still there. Note that this approach would be against some distributions policy, like Debian https://www.debian.org/doc/debian-policy/ch-opersys.html 9.1.1.

I understand that this would a major move for the project and that we should design a transition mechanism for users but I believe that the final goal of renaming the libraries and artifacts is a good way to go.

Thanks!

darksylinc commented 2 years ago

OK there's a few things:

The way I see it, we should support it with a CMake option:

What about plugins?

j-rivero commented 2 years ago

Hey Matias:

Thanks for answering that fast. Some inline comments:

The way I see it, we should support it with a CMake option:

* At CMake time we can select between `libOgreMain.so` vs `libOgreNextMain.so` (and `OgreMain.dll` vs `OgreNextMain.dll`)

* Like with deprecations, this option will be supported in Ogre 2.4, but should be gone in Ogre 2.5

* Ogre should warn at CMake-configure time and in the Ogre.log that it was compiled with a deprecated setting

* The only question is what should be the default

  * On Windows many users use the provided SDK. We should probably provide two SDKs in Ogre 2.4. The 2nd would be gone in 2.5 anyway
  * On Windows building from scratch should default to `OgreMainNext.dll`
  * On Linux (and Apple) we should default to `libOgreNext` for consistency with Debian

I like the plan and the option of enabling the rename via a CMake flag so we can control when to modify the default behavior and give people time to migrate with warnings. Sounds good to me to support the option in 2.4 with a deprecation warning and move the change to default in 2.5.

What about plugins?

* I saw your patch changes `OgreHlmsPbs` et al to `OgreNextHlmsPbs`. But there's not equivalent for `RenderSystem_GL`, `*_Vulkan` and `PluginFX`. I don't remember where they were installed by default. I typically prefer to put plugins in their own folder (gazebo does the same) before they're loaded dynamically at runtime. But I don't know if this complies with Distro rules.

You are right, plugins are a different thing and they can live outside of the standard library path. So we can keep the names but at a bit of risk of missing them with OGRE 1.x plugins continue to stay in there, more reduced than for libraries though. My question is, why not renaming them now that we are on this? If there are reasons to keep them, I'm fine, just wondering why not change the whole thing.

darksylinc commented 2 years ago

Eugene has suggested that rather than renaming every lib to OgreNext, just bake the version number.

e.g. libOgreMain.2.4.0.so and OgreMain.2.4.0.dll, libOgreHlmsPbs.2.4.0.so etc; without using sym links.

Thoughts?

j-rivero commented 2 years ago

Eugene has suggested that rather than renaming every lib to OgreNext, just bake the version number.

e.g. libOgreMain.2.4.0.so and OgreMain.2.4.0.dll, libOgreHlmsPbs.2.4.0.so etc; without using sym links.

Thoughts?

This is how the binary files of the libraries are now on Linux, see the list of files from ogre-1.12 package for example https://packages.debian.org/bullseye/amd64/libogre1.12.10/filelist or the ones I have packaged for OGRE-Next:

(debian-sid)jrivero@space:~$ dpkg -L libogrenextoverlay2.2.5 
/.
/usr
/usr/lib
/usr/lib/x86_64-linux-gnu
/usr/lib/x86_64-linux-gnu/libOgreNextOverlay.so.2.2.5
/usr/share
...

So it would be no change with respect to the current code. Problem seems to me that OGRE 1.x using Semantic Versioning from version 13 on, has open the door to release a potential 2.x version which will make binary lib file identical).

The other problem are the called SONAME symlinks, the ones in debian development packages (lib*-dev). They are well describe in Debian policy but a more easy way to understand them could be this article (look for "The Debian way").

This soname symlinks are related to ABI/API stability, if we are able to improve ABI/API stability in this project (see #258), the extension of these symlinks should be shorten to libOgreMain.so.X.Y or libOgreMain.so.X which bring more risk of missing projects again.

darksylinc commented 2 years ago

The other problem are the called SONAME symlinks, the ones in debian development packages (lib*-dev). They are well describe in Debian policy but a more easy way to understand them could be this article (look for "The Debian way").

This soname symlinks are related to ABI/API stability, if we are able to improve ABI/API stability in this project (see #258), the extension of these symlinks should be shorten to libOgreMain.so.X.Y or libOgreMain.so.X which bring more risk of missing projects again.

I don't really understand what the problem is :confused:

darksylinc commented 2 years ago

Oh wait, I understand now. The problem is that we're constantly switching between ABI stability mentalities.

Yes agreed, with 100% ABI stability, shared links to libOgreMain.so.2.4.x can be shortened to libOgreMain.so.2.4

What we meant by shared link we meant libOgreMain.so -> libOgreMain.so.2.4

j-rivero commented 2 years ago

Oh wait, I understand now. The problem is that we're constantly switching between ABI stability mentalities.

Sorry, I did not explain it correctly.

Yes agreed, with 100% ABI stability, shared links to libOgreMain.so.2.4.x can be shortened to libOgreMain.so.2.4

What we meant by shared link we meant libOgreMain.so -> libOgreMain.so.2.4

Yes, that is correct.

paroj commented 2 years ago

everything that should be side-by-side installable for development needs a distinct NAME - be it libOgreNextMain.so or libOgreMain24.so. SONAME is for specific versions of NAME, where the symlink libOgreMain.so -> libOgreMain.so.13.3 helps during development using the latest release. libOgreMain.so.13.2 might be installed side-by-side and used by older build of a different application. Thats why our plugins, too, carry a SONAME.

j-rivero commented 2 years ago

For reference, I've created this patch in Debian https://salsa.debian.org/ogre-team/ogre-next/-/blob/master/debian/patches/use-ogrenext-names-for-libs.patch which is only limited to the components and plugins used under UNIXes there. I can submit it in the form of the PR if that helps in someway but I'm afraid that it will require a good bunch of more work to be complete.

darksylinc commented 2 years ago

Your patch has been included (after thorough modifications and testing).

The new CMake option OGRE_USE_NEW_PROJECT_NAME will use OgreNext instead of Ogre for lib names.

Several scripts have been updated to account for this.

darksylinc commented 2 years ago

Ok so I've been testing the abi checker script on already-released versions to get good hold of ABI & API changes throughout our history.

Testing

I tested:

This gave me a good grasp of our ABI & API stability.

Results

What I found:

I skipped 2.1 since it was pointless. Too many changes. It took years to officially release. We did not care about stability, etc.

Analysis

Therefore I've been thinking: How does Semantic Version apply in our case? A C++ library that exposes so much to the public API.

For C programs it's easy:

For C++ programs that use abstractions like COM or Pimpl it is also manageable.

But for us? Even adding a variable to a class means we have to bump MINOR.

Then I realized OgreNext is stuck at 2.x. It's like... we're not really using the MAJOR value.

New version scheme for OgreNext

Therefore I propose OgreNext follows the following convention:

This would satisfy Ignition's goals as well as ours:

I understand this isn't strictly SemVer, because under SemVer API breakages always means MAJOR+1.

However due to the intricacies of C++ (and our lack of COM/Pimpl), we want to distinguish between 'easy to upgrade' and 'hard to upgrade'. Otherwise OgreNext's SemVer would almost always boil down to MAJOR+1 and PATCH+1, completely ignoring MINOR.

Under this scheme, OgreNext 2.4 would actually be released as OgreNext 3.0

The API between 2.3 and 3.0 didn't change that much, but because of C++ changes (we moved to C++11, added override everywhere, fixed tons of warnings, removed dead code) tools are picking many subtle changes as API breaking even though it's not really that breaking. IIRC abi-compliance-checker reports 67% API compatibility or so, but that number feels misleading in my opinion.

Objectively though, if the tool says <95%, then MAJOR gets bumped.

Nonetheless I think it would be great to label the next release as OgreNext 3.0 given that it starts the new convention.

Support cycle

We don't have enough resources for long support cycles. So far I've been doing fixes that can go far back to 2.1 (although often I start 2.2 nowadays) and merge all the way into master.

Doing this by hand is easy when it's only 2 or 3 branches but with this scheme we could end up with a ton of branches:

First, it's likely that we won't end up having that many MINOR branches.

Second, we should write a script to automate cascading fixes to all the upper branches (and stops when there's conflicts of course).

Third given our limited resources, branches with more active support will be linked to paid support (e.g. whatever Ignition is using) and the latest version.

I could add a table on the Github's README showing which branches are currently receiving support in terms of frequency (i.e. dropped, low, medium, high).

What about older releases?

Releases <= 2.3.x stay as is; with the old scheme (ABI often breaks, API breaks slightly).

What about master branch?

The only question remains what happens when we start development of a new version after release. i.e. after releasing OgreNext 3.0, should master be 4.0 or 3.1?

I've been giving it a hard thought and:

Developing master as 3.1 is risky becaues we will start accumulating API incompatibilities, giving us less manouverability if we need to fix 3.0 by breaking ABI while still being API compatible.

But if when the time arrives to make a new release and API compat is still very high (e.g. >= 98%?) it would make sense to release master as 3.1 instead of 4.0

After much consideration I believe that:

So... any comments? Feedback?

j-rivero commented 2 years ago

Hello Matias, thanks for working on this, some small comments:

Therefore I propose OgreNext follows the following convention:

* `PATCH` can only be bumped as long as ABI compatibility remains 100%.

* `MINOR` bumps as long as API compatibility stays >= 95% for all components and plugins, according to `abi-compliance-checker`. Measured against `MAJOR.0.0`

* `MAJOR` bumps when API compatibility is < 95%. Although MAJOR can be bumped even if compat >= 95% at the dev's discretion.

I understand the reasoning for arriving to the conclusion of going to this scheme. The only problem I can see with it is that binary stability at ABI level for third party software build on top is something black or white. You have it or you have not. ABI breakages bugs are specially hard to find since the effects are totally uncontrolled so for a maintainer of software build on top someone can not use a certain level of compatibility. Speaking as a Debian maintainer, it goes against the policy to update this kind of scheme without going through a full rebuild of all packages built on top of ogre-next.

Said that, if this proposal is what you think would work best for your users, we can adapt our packaging and build strategies to it. It will impact Ignition in the way that if there is an ABI/API change in ogre, a released version of ignition-rendering won't be able to update to it without patching.

All the rest of the proposal, the migration strategies and how to transition to this scheme sound good. Nice work.

darksylinc commented 2 years ago

I understand the reasoning for arriving to the conclusion of going to this scheme. The only problem I can see with it is that binary stability at ABI level for third party software build on top is something black or white. You have it or you have not. ABI breakages bugs are specially hard to find since the effects are totally uncontrolled so for a maintainer of software build on top someone can not use a certain level of compatibility. Speaking as a Debian maintainer, it goes against the policy to update this kind of scheme without going through a full rebuild of all packages built on top of ogre-next.

Said that, if this proposal is what you think would work best for your users, we can adapt our packaging and build strategies to it. It will impact Ignition in the way that if there is an ABI/API change in ogre, a released version of ignition-rendering won't be able to update to it without patching.

I'm not sure I understand. We won't break the ABI for PATCH bumps.

Are you saying the bumps to MINOR should also maintain ABI compatibility?

From our perspective that is pointless; if that were the case then the distinction between MINOR and PATCH becomes irrelevant and it would be the same as having only MAJOR.MINOR; where bumps to MINOR keeps 100% ABI compatibility.

j-rivero commented 2 years ago

I'm not sure I understand. We won't break the ABI for PATCH bumps.

Are you saying the bumps to MINOR should also maintain ABI compatibility?

Sorry, I was referring to MINORs yes.

From our perspective that is pointless; if that were the case then the distinction between MINOR and PATCH becomes irrelevant and it would be the same as having only MAJOR.MINOR; where bumps to MINOR keeps 100% ABI compatibility.

Understood. My comment was more about the difference with standard semver: new features added via bump in MINOR can not be handled cleanly by third party software. PATCH is fine and it is a good improvement over the current situation.

darksylinc commented 2 years ago

Sorry, I was referring to MINORs yes.

Ah cool. We're now in sync.

Understood. My comment was more about the difference with standard semver: new features added via bump in MINOR can not be handled cleanly by third party software. PATCH is fine and it is a good improvement over the current situation.

Yeah the goal is to move SONAME to MAJOR.MINOR, instead of the current MAJOR.MINOR.PATCH (Technically Semantic Version talks about API stability rather than ABI stability, those are two different beasts; which is why your comment had confused me).

So yeah, 100% ABI stability is only guaranteed with PATCH bumps; given that for MINOR bumps adding API features without breaking ABI would be almost impossible due to how Ogre was originally designed (unless a layer is added on top, e.g. like Unicode ICU has a stable C interface that wraps around its unstable C++ one, same goes for LLVM).

Changing that would require an engineering effort of a level that would simply mean it's a new project entirely. Even if we added a C layer, migrating Ignition to that layer would be a daunting task (just like migrating from UnicodeString to its C equivalent means rewriting all the code that uses UnicodeString)

j-rivero commented 2 years ago

Thanks Matias, +1 for the proposal.

paroj commented 2 years ago

adding API features without breaking ABI would be almost impossible due to how Ogre was originally designed

it is not that bad actually. I am trying to get there with Ogre1. The good thing about Ogre is that it extensively uses the factory pattern. This means most objects are passed by pointer, which frees you of thinking about stack size. Then, if you want to add new fields/ methods to a class you merely have to do it at the end of the class and it will not affect the ABI.

To ensure that this really does not break ABI, you have to "seal" the classes by making them "final".

The thing I am most skeptical about is

MINOR bumps as long as API compatibility stays >= 95%

as Jose said, from the outside this is a binary decision. Either my code still compiles with the new version or it does not. Keeping MINOR when you break API will do more harm than good IMO. On the other hand the ABI-checker is not that good when it comes to API.

Finally, my experience with other projects was that Debian will not update the package even if you keep ABI on MINOR. So it is not worth the hassle..