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

Read the download url of librdkafka from Windows environment variable #816

Merged
merged 4 commits into from
Sep 6, 2020
Merged

Read the download url of librdkafka from Windows environment variable #816

merged 4 commits into from
Sep 6, 2020

Conversation

yunnysunny
Copy link
Contributor

In Windows ,the download of the library of librdkafka may be so slowly, especially when you are in China. It may cost a long time to download it. Sometime I have to wait for several hours.
So I add an feature of reading the download url from environment variable.

@giulliano-bueno
Copy link

LGTM.

@yunnysunny
Copy link
Contributor Author

@iradul Consider this pull request?

@iradul
Copy link
Collaborator

iradul commented Jul 29, 2020

There is this BUILD_LIBRDKAFKA env variable that can be used to compile against system installed version of librdkafka. This doesn't work on Windows but I think it would be great if we could make it work and hence have consistency across platforms.

@yunnysunny
Copy link
Contributor Author

yunnysunny commented Jul 30, 2020

I read the code of binding.gyp, the BUILD_LIBRDKAFKA is only an flag to indicate whether to building the source code with the shared library installed in OS already. And my pull request aims at supplying an URL to download nuget package.
So I think it's two different thing.
Or do you mean just rename the env name DOWNLOAD_BASE_URL_LIB_RDKAFKA to BUILD_LIBRDKAFKA?

@yunnysunny
Copy link
Contributor Author

yunnysunny commented Aug 18, 2020

I have renamed DOWNLOAD_BASE_URL_LIB_RDKAFKA to BUILD_LIBRDKAFKA.

@iradul
Copy link
Collaborator

iradul commented Aug 31, 2020

Let's call this variable NODE_RDKAFKA_NUGET_BASE_URL and also please leave README file unchanged, I'll update it after I merge this PR.

@yunnysunny
Copy link
Contributor Author

done

@iradul iradul merged commit a037260 into Blizzard:master Sep 6, 2020
@yunnysunny
Copy link
Contributor Author

Expecting this merge can be published to npm. We can not bear the network in China now.

@iradul
Copy link
Collaborator

iradul commented Dec 7, 2020

@yunnysunny I just released the new version

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 this pull request may close these issues.

3 participants