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

NodeJSUtils.cmake: adding missing libraries to the build #2

Closed
wants to merge 4 commits into from
Closed

NodeJSUtils.cmake: adding missing libraries to the build #2

wants to merge 4 commits into from

Conversation

TonyRice
Copy link

I am replacing my previous pull request with this one. It excludes some things that I have in a pull request over here (eclipsesource#259)

@drywolf
Copy link
Owner

drywolf commented May 19, 2017

Thx for the PR.

The libraries that you added, from what I can tell are the ones introduced somewhere between Node.js v7.4.0 and v7.4.10.
For those I would rather look for a general solution than to have to cherry pick them based on which Node version should be used. Based on your feedback, I will make this point a higher priority in my backlog though.
For the statically linked MS C++ CRT libraries, I think I will just expose this as a cmake option, so users can decide what they want to build. I simply went with the statically linked libs, because in the original Visual Studio projects it was used in some of the configuration (but it wasn't uniformly used there either)

If you got other points that you would like to see included in this fork, please just let me know here, then I can schedule them for my backlog.

Thanks

@TonyRice
Copy link
Author

TonyRice commented May 28, 2017

Thanks for checkout out the PR. I'm actually managing my own fork that includes your cmake changes. I'm actually going to go ahead and modify this into a new pull request with fixes only (nothing that would diverge)

@drywolf drywolf closed this Jun 15, 2017
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.

2 participants