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

Add redis symlinks at the same place as the installed binaries #193

Merged
merged 17 commits into from
Apr 6, 2024

Conversation

vitahlin
Copy link
Contributor

@vitahlin vitahlin commented Apr 4, 2024

Adds a new make variable called USE_REDIS_SYMLINKS, with default value yes. If yes, then make install creates additional symlinks to the installed binaries:

  • valkey-server
  • valkey-cli
  • valkey-benchmark
  • valkey-check-rdb
  • valkey-check-aof
  • valkey-sentinel

The names of the symlinks are the legacy redis binary names (redis-server, etc.). The purpose is to provide backward compatibility for scripts expecting the these filenames. The symlinks are installed in the same directory as the binaries (typically /usr/local/bin/ or similar).

Similarly, make uninstall removes these symlinks if USE_REDIS_SYMLINKS is yes.

This is described in a note in README.md.

Fixes #147

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

I think the approach makes sense, just some minor comments

src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
@vitahlin vitahlin force-pushed the vitah-chore branch 2 times, most recently from ba81d4d to c4982d3 Compare April 4, 2024 06:29
vitahlin and others added 5 commits April 4, 2024 14:31
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
@vitahlin
Copy link
Contributor Author

vitahlin commented Apr 4, 2024

I think the approach makes sense, just some minor comments

Solved the language expression problem you suggested and the space problem in Makefile.
Thanks for your help.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Yeah, this is the behaviour we want. Some comments on details.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

One more thing: The symlinks valkey-check-rdb -> valkey-server and valkey-check-aof -> valkey-server work by conditional code in server.c looking for the exec name, i.e. which symlink was used for starting it. Please also check that it works for redis-check-rdb etc.

Code in server.c:

    /* Check if we need to start in redis-check-rdb/aof mode. We just execute
     * the program main. However the program is part of the Redis executable
     * so that we can easily execute an RDB check on loading errors. */
    if (strstr(exec_name,"valkey-check-rdb") != NULL)
        redis_check_rdb_main(argc,argv,NULL);
    else if (strstr(exec_name,"valkey-check-aof") != NULL)
        redis_check_aof_main(argc,argv);

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
@zuiderkwast zuiderkwast linked an issue Apr 4, 2024 that may be closed by this pull request
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
…t to uninstall redis symlink

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Disclaimer: The suggestions are untested. :)

src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
src/Makefile Outdated Show resolved Hide resolved
…links

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
@vitahlin
Copy link
Contributor Author

vitahlin commented Apr 5, 2024

One more thing: The symlinks valkey-check-rdb -> valkey-server and valkey-check-aof -> valkey-server work by conditional code in server.c looking for the exec name, i.e. which symlink was used for starting it. Please also check that it works for redis-check-rdb etc.

Code in server.c:

    /* Check if we need to start in redis-check-rdb/aof mode. We just execute
     * the program main. However the program is part of the Redis executable
     * so that we can easily execute an RDB check on loading errors. */
    if (strstr(exec_name,"valkey-check-rdb") != NULL)
        redis_check_rdb_main(argc,argv,NULL);
    else if (strstr(exec_name,"valkey-check-aof") != NULL)
        redis_check_aof_main(argc,argv);

I will test it next.

src/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Good. Just a few minor suggestions.

README.md Outdated Show resolved Hide resolved
src/server.c Show resolved Hide resolved
Signed-off-by: Vitah Lin <vitahlin@gmail.com>
@vitahlin
Copy link
Contributor Author

vitahlin commented Apr 6, 2024

My first PR that wasn't about typo. Thank you for your patience and help, have a nice day. 😀

@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label Apr 6, 2024
@zuiderkwast zuiderkwast merged commit ba0c93c into valkey-io:unstable Apr 6, 2024
14 checks passed
@zuiderkwast
Copy link
Contributor

Thanks @vitahlin! I edited the top comment to match what was finally implemented.

madolson added a commit to madolson/valkey that referenced this pull request Apr 7, 2024
…y-io#193)

Adds a new make variable called `USE_REDIS_SYMLINKS`, with default value
`yes`. If yes, then `make install` creates additional symlinks to the
installed binaries:

* `valkey-server`
* `valkey-cli`
* `valkey-benchmark`
* `valkey-check-rdb`
* `valkey-check-aof`
* `valkey-sentinel`

The names of the symlinks are the legacy redis binary names
(`redis-server`, etc.). The purpose is to provide backward compatibility
for scripts expecting the these filenames. The symlinks are installed in
the same directory as the binaries (typically `/usr/local/bin/` or
similar).

Similarly, `make uninstall` removes these symlinks if
`USE_REDIS_SYMLINKS` is `yes`.

This is described in a note in README.md.

Fixes valkey-io#147

---------

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
@vitahlin vitahlin deleted the vitah-chore branch April 8, 2024 03:53
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
…y-io#193)

Adds a new make variable called `USE_REDIS_SYMLINKS`, with default value
`yes`. If yes, then `make install` creates additional symlinks to the
installed binaries:

* `valkey-server`
* `valkey-cli`
* `valkey-benchmark`
* `valkey-check-rdb`
* `valkey-check-aof`
* `valkey-sentinel`

The names of the symlinks are the legacy redis binary names
(`redis-server`, etc.). The purpose is to provide backward compatibility
for scripts expecting the these filenames. The symlinks are installed in
the same directory as the binaries (typically `/usr/local/bin/` or
similar).

Similarly, `make uninstall` removes these symlinks if
`USE_REDIS_SYMLINKS` is `yes`.

This is described in a note in README.md.

Fixes valkey-io#147

---------

Signed-off-by: Vitah Lin <vitahlin@gmail.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
@furai
Copy link

furai commented Apr 25, 2024

For me this change causes issues when using TLS. When I switch off symlinks for redis then installation fails cause it can't find libhiredis_ssl.a. Could this implementation be revisited?

@zuiderkwast
Copy link
Contributor

@furai Yes, of course we can revise it. Why is libhiredis_ssl.a affected by this?

@furai
Copy link

furai commented Apr 25, 2024

I haven't had time to dig into it. What I'm doing is pretty simple, e.g.:

 make BUILD_TLS=yes USE_SYSTEMD=yes 
 make PREFIX=custom/usr USE_REDIS_SYMLINKS=no install

And then it fails with error because it can't find the aforementioned file. I was building from prepackaged version 7.2.5 available here on github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
Development

Successfully merging this pull request may close these issues.

Create "redis" symlinks for the binaries in make install
4 participants