-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
build: add missing libnode.lib and copy debug build if built #52442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
From what I see the commit 619768a is present in both this and the previous PR. How do you want to land this? as 2 PRs, or only this one as it includes the commit from the initial one? |
As you wish. I opened this separately in case you want to merge just the previous change, as the following commit here is a bit more complex and decides things like optionally including a debug build for the experimental shared build in Windows in the resulting packaged dist. |
And what would be the ideal use case for this in your opinion? Asking because in the original issue you wrote this:
And the way CI is running, Debug and Release are never built together, so effectively, |
This is for when someone wants to build his own custom shared build for embedding Node.js. It essentially adds the needed files to the built installed tree/package, so you can just use that folder, sharing it, building it locally on each machine, etc. |
Oh OK, my bad. What I understood was that you'd like this to be added to official releases (I read the quoted message badly). The scenario you mention is completely valid as people building their own custom shared build can do whatever they want eg. build both Release and Debug. |
The POC where I used this: https://github.com/segevfiner/node-embed |
PR-URL: #52442 Reviewed-By: Michael Dawson <[email protected]>
Landed in c29d53c |
So we can close both this and the previous PR? (As it was landed outside of GitHub's merge PR flow). |
oops should have closed when I landed it. |
PR-URL: #52442 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #52442 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #52442 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#52442 Reviewed-By: Michael Dawson <[email protected]>
This continues after #52277 and adds the missing libnode.lib and also copies the Debug build if it was built as described in #52281