redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.15k stars 1.8k forks source link

CMakeList: add option to not install NuGet packaging #1246

Closed yann-morin-1998 closed 5 months ago

yann-morin-1998 commented 5 months ago

The NuGet hiredis.target packaging description file is of no use on systems that are not using NuGet, like Linux systems, and the spurious presence of that file is not "clean".

Add a cmake option to allow users to disable installation of that file. As some people may have relied on that file to be installed, continue to install it by default.

yann-morin-1998 commented 5 months ago

@michael-grunder Hi Michael!

Thanks for triggerring the workflows. I've looked at the failures, but I'm not sure they are related to this change:

Let me know if I can do something to help solve those two.

yann-morin-1998 commented 5 months ago

@michael-grunder Hey Michael,

The macos workflow still fails, because the redis@7.0 formula does not exist in brew; I could only spot @7.2 (7.2.4) and @6.2 (6.2.14): https://formulae.brew.sh/formula/redis

michael-grunder commented 5 months ago

Yep,the failure is unrelated to yoru PR. I will fix CI and then get this merged.

michael-grunder commented 5 months ago

OK CI is fixed on the master branch. If you rebase against that the tests should now pass here as well.

One other question however. I understand the impulse ot not change behavior but is there any reasonable reason that Linux systems would have come to rely on this NuGet build artifact?

There is always the possibility of reasons like this but I think maybe we can just consider it a bug and not install the file by default?

yann-morin-1998 commented 5 months ago

OK CI is fixed on the master branch. If you rebase against that the tests should now pass here as well.

One other question however. I understand the impulse ot not change behavior but is there any reasonable reason that Linux systems would have come to rely on this NuGet build artifact?

Thing is, cmake is not necessarily Linux-only. I haven't touched a Windows machine in more than 20 years now, so I have absolutely no idea what is going on on that side of the universe, but it is my understanding that cmake works on Windows, so maybe some people are using the cmake on Windows to build NuGet packages there...

There is always the possibility of reasons like this but I think maybe we can just consider it a bug and not install the file by default?

I am totally fine with making the changing the default to OFF, if you prefer. Just tell me, Ill do that then repush.

Thanks!

michael-grunder commented 5 months ago

I haven't touched a Windows machine in more than 20 years now, so I have absolutely no idea what is going on on that side of the universe

I'm in the exact same boat. Literally clueless about the Windows side of things.

I did a quick bit of reading and apparently NuGet does have some cross-platform support via .NET core so I think the safest solution is to use your change as-is. I can circle back to this in the future to flip the default to OFF if it ever comes up again.

Happy to merge once the branch is rebased.

Thanks!

michael-grunder commented 5 months ago

Merged, thanks!