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

Fix cmake config path on Linux. #989

Merged
merged 1 commit into from
Sep 1, 2022
Merged

Conversation

xkszltl
Copy link
Contributor

@xkszltl xkszltl commented Oct 5, 2021

CMake config files were installed to /usr/local/share/hiredis, which is not recognizable by find_package().
I'm not sure why it was set that way.
Given the commit introducing it is for Windows, I keep that behavior consistent there, but fix the rest.

CMake config files were installed to `/usr/local/share/hiredis`, which is not recognizable by `find_package()`.
I'm not sure why it was set that way.
Given the commit introducing it is for Windows, I keep that behavior consistent there, but fix the rest.
@michael-grunder michael-grunder self-assigned this Oct 5, 2021
@michael-grunder
Copy link
Collaborator

I'm pretty unfamiliar with CMake but this does appear correct. I'll read up a bit on CMake's find_package and then get it merged.

@xkszltl
Copy link
Contributor Author

xkszltl commented Oct 6, 2021

Emmm....Actually after double checking I might be wrong. /usr/local/share/hiredis could be supported as well. Originally I saw it after packaging into rpm and think that doesn't look right, probably because rarely anyone uses that.

image

@michael-grunder
Copy link
Collaborator

So can this be closed then?

@xkszltl
Copy link
Contributor Author

xkszltl commented Dec 23, 2021

Up to you, but I would recommend to merge.
This PR fixed a "package not found" issue, although I'm not sure about the root cause because either way should be OK based on the doc.

Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

Looks good - and even tested in a Windows VM and Linux docker.

@michael-grunder
Copy link
Collaborator

Merged, thanks!

@chayim Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants