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

Clean up library VERSION and SOVERSION #1069

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

cary-ilm
Copy link
Member

Reduce confusion between "VERSION", "REVISION", and "SOVERSION":

  • Label the internal variable IMATH_LIBTOOL_* to indicate their purpose.
  • Use terminology closer to the libtool description
  • Add comment documenting the library update process

Signed-off-by: Cary Phillips cary@ilm.com

Reduce confusion between "VERSION", "REVISION", and "SOVERSION":
* Label the internal variable IMATH_LIBTOOL_* to indicate their purpose.
* Use terminology closer to the libtool description
* Add comment documenting the library update process

Signed-off-by: Cary Phillips <cary@ilm.com>
Comment on lines +41 to +49
# When updating:
# 1. no API change: CURRENT.REVISION+1.AGE
# 2. API added: CURRENT+1.0.AGE+1
# 3. API changed: CURRENT+1.0.0
#
set(OPENEXR_LIBTOOL_CURRENT 29)
set(OPENEXR_LIBTOOL_REVISION 0)
set(OPENEXR_LIBTOOL_AGE 0)
set(OPENEXR_LIB_VERSION "${OPENEXR_LIBTOOL_CURRENT}.${OPENEXR_LIBTOOL_REVISION}.${OPENEXR_LIBTOOL_AGE}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hear me out on this kooky idea:

Our version numbers are MAJOR.MINOR.PATCH. We assume that even minor releases change API, but that we don't change API for patch releases, right?

What if CURRENT = MAJOR*100+MINOR, and REVISION=0 (by default), and AGE = PATCH? Then the vast majority of time, we wouldn't need to adjust anything at all for a release. And very very occasionally, if a patch release necessitated adding an API call, we would bump AGE in that release branch.

The advantage to making this automated based on the release version is that it's much harder to make mistakes. If these numbers are all varying independently of release version and changed by hand, we have to be much more careful about bumping soversion on every release and doing it correctly, remembering which kinds of changes got incorporated into that release. Frankly, I don't trust us to do it right, unless we have the CI include a fully automated and foolproof test that confirms that API doesn't change without these numbers being bumped properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I said it wrong. I meant,

What if CURRENT = MAJOR*100+MINOR, and REVISION=0 (by default), and AGE = PATCH? Then the vast majority of time, we wouldn't need to adjust anything at all for a release. And very very occasionally, if a patch release necessitated adding an API call, we would bump REVISION in that release branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, since the name of the library contains the major and minor releases, CURRENT can always be 0 for all we care.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, that's not quite true. If we ever have a patch release that adds an API call (do we? would we?), then we would need to bump current. I find this scheme maddeningly easy to misunderstand or to do it incorrectly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 2.5.x series is in the category, isn't it? Patch releases with API changes.

@cary-ilm
Copy link
Member Author

I'm bringing this up for the exact reason you cite, I strongly suspect we're not updating these numbers properly, because it's so confusing. I'm certainly open to other ideas or solutions.

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

I like the idea of adding the word LIBTOOL in there, so we know what this weird thing is. I also like Larry's idea of making the libtool numbers a procedural derivative of the numbers we care about. Unless someone who is confident about libtool speaks up, I'd be fine with current being major*10000 + minor * 100 + patch, and the other two values being zero. I just am not really spotting a deeper value in the libtool scheme from our perspective, at all... I imagine the purpose is that linux distributions can keep an old exr hanging around if age changes, but need to relink if revision or current changes. To be happy with a more complex scheme than a simple current hash, I'd like to know under what practical circumstances age and revision provide upstream value. I just am not wrapping my brain around it.

@lgritz
Copy link
Contributor

lgritz commented Jul 2, 2021

I think you want the patch to be reflected in the age, not current, because it's ok to substitute a new patch release, right?

@meshula
Copy link
Contributor

meshula commented Jul 2, 2021

I believe that's true, I'm just wondering if that's a real use case? If someone is releasing a new ubuntu, it's unlikely they're going to cleverly avoid building openexr. If it's you or I building for our own not /usr/local directory, why are we doing that? If it's to rev an app like Blender, recompiling exr is 1% of 1% of the build time. So what I'm questioning is whether age is a pedantic correctness thing as opposed to a practical correctness. If it's merely pedantic, I'm all for all sails to the wind, let's do the thing that requires the least brain power, and only bump one number, because right now I'm feeling like maybe it's only an issue for PedanticKitty :) ... I am looking to be educated here, I feel like we've been coddling this problem along for almost plural decades now and still confused about it.

@lgritz
Copy link
Contributor

lgritz commented Jul 2, 2021

I don't disagree, as far as my personal workflow and priorities. I don't know if anybody else really cares.

I will note that if we wrap the whole thing up in current, we can never make a substitution that breaks (that's the single most important constraint), and although it doesn't allow for some possibly correct substitutions, it's no worse than the situation people would be in if they were linking statically.

@cary-ilm
Copy link
Member Author

Raising this again, as we need to resolve it before the 3.1 release.

In spite of how much I'd like to (a) not think about this again, and (b) not make mistakes, I'm not sure the automatic formula conforms to the libtool instructions: https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html, which also say: "Never try to set the interface numbers so that they correspond to the release number of your package. This is an abuse that only fosters misunderstanding of the purpose of library versions." Since the project has traditionally followed this library versioning policy, I'd prefer to stick with it, and just be deliberate.

The 3.0.5 release has library version 29.0.0. The 3.1 release make a few minor internal fixes (most have been patched into 3.0.5) and adds the OpenEXRCore library. So:

  1. Start with version information of ‘0:0:0’ for each libtool library.
  2. Update the version information only immediately before a public release of your software.
  3. If the library source code has changed at all since the last update, then increment revision (‘c:r:a’ becomes ‘c:r+1:a’).
    yes -> 29.1.0
  4. If any interfaces have been added, removed, or changed since the last update, increment current, and set revision to 0.
    yes -> 30.0.0
  5. If any interfaces have been added since the last public release, then increment age.
    yes -> 30.0.1
  6. If any interfaces have been removed or changed since the last public release, then set age to 0.
    no -> 30.0.1

Sound right?

@lgritz
Copy link
Contributor

lgritz commented Jul 13, 2021

Since we have an entirely different library name for every minor release (libOpenEXR-3.1.so vs libOpenEXR-3.2.so), do we start at 0:0:0 again for every one of those? Or does 3.2 pick up at where 3.1 left off?

@lgritz
Copy link
Contributor

lgritz commented Jul 13, 2021

I'm not sure your examples are correct. I don't think you can ever have a x.0.1 because age says how far BACK in revision is considered compatible, and there is no revision prior to 0.

This is so hard to get right.

@cary-ilm
Copy link
Member Author

We also install libOpenEXR.so in the chain of symlinks to libOpenEXR-3_0.29.0.0, and I think that's where the "drop-in" policy figures in, so I don't think the -3_1 suffix resets things.

@meshula
Copy link
Contributor

meshula commented Jul 14, 2021

And as pointed out in the new issue, drop-in of arbitrary OpenEXR libs into a cascade of symlinks such that one can simply link libOpenEXR.so doesn't work anyway because the version is burned into the ABI.

@cary-ilm
Copy link
Member Author

I'd like propose that we go with this PR as is for now. It doesn't change behavior or policy, it simply names variables in a slightly less confusing way. We approved similar changes for Imath, which went into 3.1. I don't think we should hold up the 3.1 release of OpenEXR in hopes of resolving this any more effectively, although I'm open to ideas.

Can someone approve the review so I can merge it?

@cary-ilm cary-ilm merged commit 51c1f7b into AcademySoftwareFoundation:master Jul 15, 2021
@cary-ilm cary-ilm added the v3.1.0 label Sep 2, 2021
@cary-ilm cary-ilm deleted the lib-version branch November 5, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants