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

CMakeList: add option to not install NuGet packaging #1246

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

yann-morin-1998
Copy link
Contributor

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
Copy link
Contributor Author

@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:

  • macOS workflow: the failure happens in the job setup, while
    downloading the dependencies;

  • BSD workflow: it looks like the test system in the VM is constantly
    reoobting (or retarting) even before starting the build, and the VM
    was killed after 6 hours.

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

@yann-morin-1998
Copy link
Contributor Author

@michael-grunder Hey Michael,

The macos workflow still fails, because the [email protected] 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
Copy link
Collaborator

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

@michael-grunder
Copy link
Collaborator

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?

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.

Signed-off-by: Yann E. MORIN <[email protected]>
@yann-morin-1998
Copy link
Contributor Author

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
Copy link
Collaborator

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 michael-grunder merged commit ff7a064 into redis:master Jan 21, 2024
12 checks passed
@michael-grunder
Copy link
Collaborator

Merged, thanks!

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

Successfully merging this pull request may close these issues.

2 participants