Skip to content

Upstream patch from Cygwin package repo#191

Merged
bjosv merged 5 commits intovalkey-io:mainfrom
bjosv:cygwin-patch
Apr 25, 2025
Merged

Upstream patch from Cygwin package repo#191
bjosv merged 5 commits intovalkey-io:mainfrom
bjosv:cygwin-patch

Conversation

@bjosv
Copy link
Copy Markdown
Collaborator

@bjosv bjosv commented Apr 11, 2025

Repology showed that there already is a Cygwin package, which required a patch to build.

Add the corrections from the patch:

  • fix the valkey_tls linkage on Cygwin to avoid an build error when building with TLS.
  • Set the SOVERSION needed for Cygwin builds.
    The built library will be named "cygvalkey-0.1.dll" instead of "cygvalkey.dll".

Additionally, build Cygwin using CMake in its own job.
Build Cygwin using CMake only, which is the supported alternative on Windows.

NOTE:
The addition of SOVERSION will also set the compatibility version field in macOS:

BEFORE:
# otool -L libvalkey.dylib
    libvalkey.dylib:
            @rpath/libvalkey.0.1.dylib (compatibility version 0.0.0, current version 0.1.0)

AFTER:
# otool -L libvalkey.dylib
    libvalkey.dylib:
            @rpath/libvalkey.0.1.dylib (compatibility version 0.1.0, current version 0.1.0)

But this seems ok, and not used in later macos release: https://github.com/giordano/macos-compatibility-version

bjosv added 3 commits April 11, 2025 10:52
Replace the current Cygwin build with CMake,
which is the supported alternative on Windows.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
This change will build "cygvalkey-0.1.dll" instead of "cygvalkey.dll" on Cygwin.
It will also set the compatibility version field in macOS:

> otool -L libvalkey.dylib
libvalkey.dylib:
	@rpath/libvalkey.0.1.dylib (compatibility version 0.0.0, current version 0.1.0)
libvalkey.dylib:
	@rpath/libvalkey.0.1.dylib (compatibility version 0.1.0, current version 0.1.0)

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 11, 2025

@michael-grunder maybe you know more about the compatibility version i Mac-libs (Mach-O)?

@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 11, 2025

/cc @fd00 Does this change seem ok? Nothing more needed?

@michael-grunder
Copy link
Copy Markdown
Collaborator

maybe you know more about the compatibility version i Mac-libs (Mach-O)

I don't know very much about them but I think the change is correct. AFAIK the first triplet is "compatibility version" and the second "current version". You bump the current version each release but only bump compatibility version if there is a breaking change.

Applications embed this compatibility version at link time and then check it each time the application starts. If it detects a compatibility version that is greater than the one embedded in the binary it will refuse to start.

@fd00
Copy link
Copy Markdown

fd00 commented Apr 12, 2025

Thank you for the notification.

We have only confirmed that we can build on cygwin by applying this patch.

Some of the ctest results I have run locally have not confirmed success.
(That's why we are assuming it is WIP.)

As for soversion, the cmake variable name contained SONAME, so we applied it as is.

bjosv added 2 commits April 22, 2025 10:53
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 23, 2025

Some of the ctest results I have run locally have not confirmed success. (That's why we are assuming it is WIP.)

I added a step to run some tests on Cygwin as well, but it uses memurai which is valkey/redis windows alternative.
I haven't seen that Cygwin provides a pre-built packages for redis/valkey to be able to run ctest, but this was the easiest option.
I found and fixed one failing test which might be what you have seen.

@michael-grunder
Copy link
Copy Markdown
Collaborator

but it uses memurai

I switched to memurai for hiredis Windows tests a few years ago. Hasn't caused any issues that I'm aware of.

@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Apr 25, 2025

I switched to memurai for hiredis Windows tests a few years ago. Hasn't caused any issues that I'm aware of.

Yes, it works fine. It was mentioned that ctest doesn't work and this is because CMake creates a test target that uses ctest, which in turn runs our test.sh that doesn't work on Cygwin with memurai.
Running the test-binary independently towards the already running memurai instance will work fine for now I hope.

@bjosv bjosv merged commit 55ac3e4 into valkey-io:main Apr 25, 2025
47 checks passed
@bjosv bjosv deleted the cygwin-patch branch April 25, 2025 09:24
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

Successfully merging this pull request may close these issues.

3 participants