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

build: add missing libnode.lib and copy debug build if built #52442

Closed
wants to merge 2 commits into from

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Apr 9, 2024

This continues after #52277 and adds the missing libnode.lib and also copies the Debug build if it was built as described in #52281

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform. labels Apr 9, 2024
@segevfiner segevfiner changed the title Windows-node-shared-fixes build: add missing libnode.lib and copy debug build if built Apr 9, 2024
Copy link
Member

@mhdawson mhdawson left a 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.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2024
@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor

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?

@segevfiner
Copy link
Contributor Author

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.

@StefanStojanovic
Copy link
Contributor

And what would be the ideal use case for this in your opinion? Asking because in the original issue you wrote this:

Support building a package from vcbuild.bat for a debug build. Alternatively, we can also just include a Debug directory under the normal release package with the node shared library, import library and def file for debug if debug is built in that tree to produce one package that includes both release and debug builds for the shared node case.

And the way CI is running, Debug and Release are never built together, so effectively, ..\Debug will never exist.

@segevfiner
Copy link
Contributor Author

shared is also not built in CI AFAIK?

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.

@StefanStojanovic
Copy link
Contributor

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.

@segevfiner
Copy link
Contributor Author

The POC where I used this: https://github.com/segevfiner/node-embed

mhdawson pushed a commit that referenced this pull request Apr 25, 2024
PR-URL: #52442
Reviewed-By: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member

Landed in c29d53c

@segevfiner
Copy link
Contributor Author

So we can close both this and the previous PR? (As it was landed outside of GitHub's merge PR flow).

@mhdawson
Copy link
Member

oops should have closed when I landed it.

@mhdawson mhdawson closed this Apr 29, 2024
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
PR-URL: #52442
Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52442
Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52442
Reviewed-By: Michael Dawson <midawson@redhat.com>
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
PR-URL: nodejs#52442
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants