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

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

Open
j-rivero opened this issue Jan 12, 2022 · 17 comments
Open

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

j-rivero opened this issue Jan 12, 2022 · 17 comments

Comments

@j-rivero
Copy link
Contributor

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
Copy link
Member

OK there's a few things:

  • Renaming libOgreMain to libOgreNextMain (incl Windows) would break a lot of projects using Ogre, but it is easily fixed
  • OgreNext 2.4 already has quite a lot of breaking changes so we could say "why not?"
  • But...
    • For many users these other changes would result in warnings, rather than build errors
    • For another portion of users, the build did break because e.g. include paths are now in OGRE-Next instead of OGRE/ folder

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

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.

@j-rivero
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

darksylinc commented Jan 12, 2022

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 😕

@darksylinc
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

paroj commented Jan 13, 2022

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.

@paroj paroj changed the title Renaming libraies in OGRE-Next: libOgre -> libOgreNext Renaming libraries in OGRE-Next: libOgre -> libOgreNext Jan 13, 2022
@j-rivero
Copy link
Contributor Author

j-rivero commented Feb 2, 2022

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
Copy link
Member

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.

  • OGRE_USE_NEW_PROJECT_NAME will be off by default in OgreNext 2.4
  • OGRE_USE_NEW_PROJECT_NAME will be on by default in OgreNext 2.5
  • OGRE_USE_NEW_PROJECT_NAME is scheduled for removal in OgreNext 2.6

@darksylinc
Copy link
Member

darksylinc commented Apr 3, 2022

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:

  • 2.2.n -> 2.2.n+1 (e.g. 2.2.0 -> 2.2.1; 2.2.1 -> 2.2.2 and so on)
  • 2.2.7 -> 2.3.0
  • 2.3.0 -> 2.3.1
  • 2.3.1 -> 2.4.0 (aka current master)

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

Results

What I found:

  • 2.2.0 had the most new features since 2.1 (the texture refactor), thus ABI breakage was most common from 2.2.0 -> 2.2.1 and 2.2.1 -> 2.2.2 due to feedback and bugs reported by our users. But after that it stabilized
  • Several 2.2.x ABI breakages perhaps could've been avoided if we cared about it
    • Eg. changes like void foo( int a ) became void foo( int a, bool newParam ) could perhaps be fixed by having foo( int a ) call foo( int a, bool newParam ) and mark it as deprecated.
  • All 2.2.x had > 95% API compatibility. Often it was 100% compatibility
    • Not surprising but reassuring 😄. Since I've been doing an effort in which users updating MINOR releases only have to do minor work to rebuild.
  • Biggest ABI breaking change since 2.2.2 in the 2.2 branch was the EGL headless feature.
    • We did that as an exception.
    • However API compatibility was still very high.

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:

  • Function signature stays the same then MINOR version stays the same. PATCH += 1
  • New functions are added, MINOR += 1
  • Functions change signature, or functions are removed, MAJOR += 1

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:

  • 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.

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

  • Patch +1 bumps can be release for hot fixes. Bugs that are easy to fix without breaking ABI and deploy everywhere.
  • Minor +1 tells our users upgrading is painless. A recompilation is necessary. Build errors may appear, but they should be very quick & easy to fix.
    • Projects like Ignition which have strong ABI requirements can leverage whether to move to Minor+1 if they have to when a certain bugfix is too important and can't be done without breaking OgreNext's ABI.
    • Main focus for minor bumps will be for fixing bugs that can't be or are too hard to fix without breaking ABI.
  • Major +1 tells our users to prepare for larger upgrading efforts. Although not necessarily too large if we release more often.

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:

  • 3.0
  • 3.1
  • 3.2
  • 3.3
  • 4.0
  • 4.1
  • 5.0

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:

  • By default master should always bump MAJOR, i.e. if we release OgreNext 3.0, master should be 4.0
  • When the time comes to release, we can always change this and branch to 3.1
    • At our discretion, we reserve the right to still release it as 4.0, instead of 3.1
    • The rationale behind this, is that there are releases where new features are introduced that may be unstable and will need a lot of testing. Therefore a lot of fixes are anticipated and reserving 3.1 makes sense
    • As an example, this is what happened with OgreNext 2.2, where we released 2.2.0 through 2.2.6 because there was a major texture refactor, which required a lot of minor tweaking and fixing.
    • Other causes may be a paying client anticipating they'll need several minor features or fixes

So... any comments? Feedback?

@j-rivero
Copy link
Contributor Author

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
Copy link
Member

darksylinc commented Apr 18, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Thanks Matias, +1 for the proposal.

@paroj
Copy link
Member

paroj commented Apr 19, 2022

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..

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

No branches or pull requests

3 participants