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

Version 1.2.0 #1202

Merged
merged 5 commits into from
Jul 12, 2023
Merged

Version 1.2.0 #1202

merged 5 commits into from
Jul 12, 2023

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Jul 2, 2023

No description provided.

michael-grunder
michael-grunder previously approved these changes Jul 4, 2023
Copy link
Collaborator

@michael-grunder michael-grunder left a comment

Choose a reason for hiding this comment

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

This is good, just minor define/date touchups.

ABI compatibility is 100% so there are no issues there.

hiredis-compat-report.html.zip

CHANGELOG.md Outdated Show resolved Hide resolved
hiredis.h Outdated Show resolved Hide resolved
@michael-grunder
Copy link
Collaborator

michael-grunder commented Jul 4, 2023

cc @yossigo Do you mind weighing in on whether or not we should bump the SONAME here? v1.2.0 has a new async adapter, and one new redisConnectWithOptions option, but is 100% compatible (API and ABI) with v1.1.0.

Let me know if I'm missing something, but I think we can maintain the SONAME at v1.1.0 so downstream packages that currently link with it don't need to be rebuilt.

@chayim Once we iron that detail out we can move forward with a release.

yossigo
yossigo previously approved these changes Jul 4, 2023
Copy link
Member

@yossigo yossigo left a comment

Choose a reason for hiding this comment

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

@michael-grunder The right thing to do is bump the minor version to indicate we've extended the API/ABI in a backward-compatible manner.

In the case of Linux, this shouldn't require recompilation because shared object dependencies are expressed using the major version alone. IIRC, some other platforms do link against a specific minor version but have a SemVer-aware dynamic linker that will pick newer minor versions.

@michael-grunder
Copy link
Collaborator

Thanks @yossigo. The reason I ask is because I ran into this before (see #990). I'm just worried that we'll immediately get cranky comments from package maintainers. 😄

@yossigo
Copy link
Member

yossigo commented Jul 4, 2023

@michael-grunder The complaint was about changing SONAME for no reason since this was an internal fix with no ABI impact. Now, we are extending the ABI, so bumping the minor versions seems justified and required.

BTW, while technically it was not necessary to change the version in 1.0.1, my understanding is that it should not have a negative impact on any system that assumes versioning adheres to SemVer. But I guess this is implementation specific and, on some platforms, may come with a price.

Maybe @jfeep or @rgacogne can chime in and share their perspective with us.

@rgacogne
Copy link

rgacogne commented Jul 5, 2023

I'm not an expert on the subject at all, but my understanding is that the SONAME is about backward-compatibility, and nothing else, so in my humble opinion it should only be bumped when an existing symbol is removed, or the signature of an existing symbol changed. The Debian documentation 1 seems to agree:

Every time the shared library ABI changes in a way that could break binaries linked against older versions of the shared library, the SONAME of the library and the corresponding name for the binary package containing the runtime shared library should change. Normally, this means the SONAME should change any time an interface is removed from the shared library or the signature of an interface (the number of parameters or the types of parameters that it takes, for example) is changed. This practice is vital to allowing clean upgrades from older versions of the package and clean transitions between the old ABI and new ABI without having to upgrade every affected package simultaneously.

The SONAME and binary package name need not, and indeed normally should not, change if new interfaces are added but none are removed or changed, since this will not break binaries linked against the old shared library.

Now in theory changing the minor version could be done without triggering a rebuild of binaries linked against libhiredis.so, but only if the binaries have been linked against the libhiredis.so.MAJOR version (this can be checked by looking at the NEEDED section in the binary via, for example, objdump -p <binary> | grep -F NEEDED | gerp -F libhiredis). Right now it looks like hiredis's Makefile does not generate the symbolic link for the MAJOR SONAME, but only for the MINOR:

        $(INSTALL) $(DYLIBNAME) $(INSTALL_LIBRARY_PATH)/$(DYLIB_MINOR_NAME)
        cd $(INSTALL_LIBRARY_PATH) && ln -sf $(DYLIB_MINOR_NAME) $(DYLIBNAME)

I have not looked at what other distributions are doing, but on Arch it means we currently have /usr/lib/libhiredis.so and /usr/lib/libhiredis.so.1.1.0, and packages are linked against /usr/lib/libhiredis.so.1.1.0 and will require a rebuild for the slightest SONAME change. For example for Unbound:

$ objdump -p /usr/bin/unbound | grep -F NEEDED | grep -F libhiredis
  NEEDED               libhiredis.so.1.1.0 

Therefore I would personally prefer the SONAME to remain untouched, since the ABI remains fully backward-compatible, and I would suggest adding a symbolic link for libhiredis.so.MAJOR in the next release so you have more options in later releases. But I will of course do the rebuild of affected Arch packages if needed :)

with minor too

Co-authored-by: Michael Grunder <[email protected]>
@chayim chayim dismissed stale reviews from yossigo and michael-grunder via 9832a3a July 6, 2023 06:10
@chayim
Copy link
Contributor Author

chayim commented Jul 6, 2023

@rgacogne Happy to revert the SONAME side. To be fair, I thought that was part and parcel of these minor bumps. Reverted SONAME here back to 1.1.1-dev to match.

hiredis.h Outdated Show resolved Hide resolved
@michael-grunder
Copy link
Collaborator

Thanks @rgacogne that's very useful info.

I vote we maintain the SONAME at 1.1.0. Unless there are any objections we can do that and then tag.

yossigo added a commit to yossigo/hiredis that referenced this pull request Jul 6, 2023
This change addresses the issue discussed in redis#1202 and should make it
possible in the future to update minor versions without requiring
re-linking binaries.
@yossigo
Copy link
Member

yossigo commented Jul 6, 2023

Thanks @rgacogne, I actually did not realize hiredis does not symlink the major version - this makes a big difference. I agree we should avoid bumping the minor version in this case, and created #1204 to help with that in the future.

chayim and others added 2 commits July 9, 2023 14:53
Co-authored-by: Michael Grunder <[email protected]>
Co-authored-by: Michael Grunder <[email protected]>
michael-grunder pushed a commit that referenced this pull request Jul 10, 2023
This change addresses the issue discussed in #1202 and should make it
possible in the future to update minor versions without requiring
re-linking binaries.
@michael-grunder
Copy link
Collaborator

@chayim We should make sure #1204 gets into v1.2.0

@chayim
Copy link
Contributor Author

chayim commented Jul 10, 2023

@michael-grunder I see it just got merged... Assuming we're good with this, and there are no changes as discussed...

  1. approve + merge
  2. release-drafter -> tag

WDYT?

@chayim
Copy link
Contributor Author

chayim commented Jul 11, 2023

@michael-grunder last bit of this, to ensure no scope creep. Confirming merge of #1205 in here (please thumbs up/down) and then that gets merged, and this plan followed.

Thumbs down if you disagree with the merge proposal, and then we release as is. Last step @yossigo before release.

@chayim chayim merged commit 60e5075 into redis:master Jul 12, 2023
@michael-grunder michael-grunder mentioned this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants