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] Install fails with BUILD_TLS=yes, USE_REDIS_SYMLINKS=no #377

Closed
zuiderkwast opened this issue Apr 25, 2024 · 8 comments · Fixed by #382
Closed

[BUG] Install fails with BUILD_TLS=yes, USE_REDIS_SYMLINKS=no #377

zuiderkwast opened this issue Apr 25, 2024 · 8 comments · Fixed by #382
Assignees

Comments

@zuiderkwast
Copy link
Contributor

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.

Originally posted by @furai in #193 (comment)

Installation fails cause it can't find libhiredis_ssl.a.

michael-grunder added a commit to michael-grunder/valkey that referenced this issue Apr 25, 2024
@michael-grunder
Copy link

michael-grunder commented Apr 25, 2024

The problem is that the BUILD_TLS env var being pulled from .make-settings doesn't propagate to the dep build.

It's fixable by appending this after -include .make-settings in src/Makefile:

export BULD_TLS

I'm not sure if that's the right/clean way to fix it though.

@zuiderkwast
Copy link
Contributor Author

@michael-grunder What does this has to do with USE_REDIS_SYMLINKS=no?

@michael-grunder
Copy link

michael-grunder commented Apr 25, 2024

@zuiderkwast Nothing, I don't think anyway.

The build was failing because the second invocation of make rebuilt hiredis but without forwarding BUILD_TLS=yes to the dep makefile so Valkiey builds expecting libhiredis_ssl.a but hiredis doesn't build it.

Making the above change fixes the linker error. Isn't USE_REDIS_SYMLINKS just so we symlink redis-server instead of valkey-server, etc?

@zuiderkwast
Copy link
Contributor Author

Isn't USE_REDIS_SYMLINKS just so we symlink redis-server instead of valkey-server, etc?

Yes, but it seemed to be part of the problem.

@furai Do you get the same error without USE_REDIS_SYMLINKS=no?

@michael-grunder
Copy link

michael-grunder commented Apr 25, 2024

Do you get the same error without USE_REDIS_SYMLINKS=no

No that also solves the problem which does seem weird.

Interestingly when you include USE_REDIS_SYMLINKS=no the second build does far more work. Basically a complete rebuild without respect to the previously built deps. Perhaps this is actually related to the .make-settings cache.

@zuiderkwast
Copy link
Contributor Author

Interestingly when you include USE_REDIS_SYMLINKS=no the second build does far more work. Basically a complete rebuild without respect to the previously built deps. Perhaps this is actually related to the .make-settings cache.

Yeah, that's it! That USE_REDIS_SYMLINKS variable sets some macro in CFLAGS! We can remove that. We don't need to set that macro in CFLAGS.

See here: https://github.com/valkey-io/valkey/pull/193/files#diff-1abc5651133d108c0c420d9411925373c711133e7748d9e4f4c97d5fb543fdd9R7008

@zuiderkwast
Copy link
Contributor Author

@furai Workaround: Use USE_REDIS_SYMLINKS=no in make and in make install.

@zuiderkwast zuiderkwast self-assigned this Apr 25, 2024
zuiderkwast added a commit that referenced this issue Apr 25, 2024
Don't let the Make valiable `USE_REDIS_SYMLINKS` affect the build.
If it does, it causes the second line in the example below (`make
install`) to recompile what was already compiled on the line above, and
this time it's built without BUILD_TLS=yes USE_SYSTEMD=yes.

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

Fixes #377

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@furai
Copy link

furai commented Apr 26, 2024

Workaround worked for me, thanks!

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 a pull request may close this issue.

3 participants