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

[BUG] Wrong version number #392

Open
furai opened this issue Apr 26, 2024 · 9 comments
Open

[BUG] Wrong version number #392

furai opened this issue Apr 26, 2024 · 9 comments

Comments

@furai
Copy link

furai commented Apr 26, 2024

Describe the bug

Valkey reports wrong version number.

To reproduce

  1. Compile valkey from 7.2.5 tag.
  2. Do valkey-cli --version (or any other compiled library).
  3. Result is 7.2.4

Expected behavior

It should return 7.2.5.

Additional information

I've noticed this while working on my AUR package. Maybe I'm compiling something wrong. Here's link to the package for reference: https://aur.archlinux.org/packages/valkey and https://aur.archlinux.org/packages/valkey-git

@Shivshankar-Reddy
Copy link
Contributor

Thanks for reporting the issue!
Looks like valkey-cli's cliVersion api is not updated with VALKEY_VERSION.
image

@Shivshankar-Reddy
Copy link
Contributor

@zuiderkwast @madolson 7.2.5 code base looks different, for instance cliVersion definition available in valkey-cli.c and in unstable its available in cli_common.c file. Because this in-consitency valkey changes not appled otherwise should be fixed in #232

@madolson
Copy link
Member

Yeah, the code changed quite a bit between 7.2 and unstable, and we were trying to be very judicious about the backport. I don't think we thought about this one. It probably makes sense to update this to be the server_version instead of the redis version though.

@furai
Copy link
Author

furai commented Apr 26, 2024

For me personally as a new user not related to the project it makes more sense to have 7.2.5 version everywhere when I compile valkey. I'm thinking from a perspective where no prior knowledge of redis is assumed, like totally new person to the topic. For them it would be confusing (and for me in this instance as well) that when they check what version of software they're using and they're getting different number than expected.

Same goes for the default decision to have redis-cli and what not symlinked. These in my opinion shouldn't be symlinked by default. On top of that, because of that symlink, both redis and valkey can't coexist on the system cause the install locations conflict.

@hwware
Copy link
Member

hwware commented Apr 26, 2024

@furai Since you a new user for our Valkey, I just have one question. Do you feel confused as the following highlight part in json file?

image

I am not sure, Welcome feeback. Thanks

@furai
Copy link
Author

furai commented Apr 26, 2024

You mean that it's 6.0.0 when the project was still redis? That's something for documentation, right? I mean, it's kept for historical reasons, you can go in this repo and find 6.0.0 version. That's versioning of the software and it makes perfect sense. The naming redis/valkey is just a sprinkle on top and I wouldn't touch that. Definitely I wouldn't prepend 6.0.0 with redis word. That would be even more confusing.

I'd for sure wouldn't want to mix versioning and "branding". For me it doesn't matter that this previously was named redis. All I care about is knowing that it was introduced at some point in the past, certain version.

As to my original issue - we have released version 7.2.5 and it wasn't reflected in --version for certain binaries. I don't think it should state that it was redis-7.2.4 as this would be even more confusing. I get that feature-wise it was on par with 7.2.4 version of redis but that's totally irrelevant for someone like me.

That's just my personal feeling about the whole issue.

@zuiderkwast
Copy link
Contributor

@furai You're right, this is confusing. I'm not sure what we need to do with exactly this bug. Maybe we can fix it and release it as 7.2.6.

So why is it like this? Our 7.2.x is a "redis compatibility release". We were trying to keep everything "redis 7.2.4 compatible" and pretend that we are Redis. It makes no sense for you but it can be important for other users. Some commands return error message like "Redis is busy" and it was important to keep things like that, because some clients and tools are hard-coded to check for exactly these strings.

In 8.0 we will have a much better situation. We will have a extended-redis-compatiblity config, default off. When it's on, valkey will pretend to be Redis 7.2.4 and when it's off (by default) it will be Valkey 8.0.0.

Same goes for the default decision to have redis-cli and what not symlinked. These in my opinion shouldn't be symlinked by default. On top of that, because of that symlink, both redis and valkey can't coexist on the system cause the install locations conflict.

The reason is that some tools and user scripts assume that there are commands like redis-cli, so to be redis-compatible we should provide them. It's possible to disable these when installing.

You have the same problem when installing two different versions of valkey or two different versions of redis. It's possible to install them in different directories. Another possibility is to add a suffix to the binaries' names, make PROG_SUFFIX="-foo" will compile binaries with names like valkey-cli-foo.

@madolson
Copy link
Member

madolson commented May 1, 2024

So I guess we are saying that for 7.2, it will stay compatible with Redis, then when release Valkey 8.0 it will reflect the valkey version?

@zuiderkwast
Copy link
Contributor

Yes, in Valkey 8 we will distinguish between the Valkey version and the Redis compat version and return one or the other in various places.

But in Valkey 7.2.x we don't distinguish between valkey version and redis-compat-version.

If we make a patch for this in the 7.2 branch, it probably doesn't harm to return 7.2.5 or 7.2.6 here. Although there is not yet any such Redis OSS version, if there will be, it won't have any new features or breaking changes.

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

5 participants