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,win: rename node.lib to libnode.lib #27150

Merged
merged 1 commit into from
Apr 19, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 9, 2019

eliminate the need for rename_node_bin_win

P.S. This might be considered semver-major, but IMO only in the context of of @nodejs/delivery-channels

/CC @nodejs/build-files @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Apr 9, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@refack refack self-assigned this Apr 9, 2019
@nodejs-github-bot
Copy link
Collaborator

@refack refack added the review wanted PRs that need reviews. label Apr 9, 2019
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is happy.

@@ -23,7 +23,7 @@
'node_v8_options%': '',
'node_enable_v8_vtunejit%': 'false',
'node_core_target_name%': 'node',
'node_lib_target_name%': 'node_lib',
'node_lib_target_name%': 'libnode',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would make the product..liblibnode.so/liblibnode.dylib on other platforms? (looks a bit funny, but probably not a big deal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would make the product..liblibnode.so/liblibnode.dylib on other platforms?

This is the line I see on linuxONE for example:

ar crsT /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/libnode.a ...
                                                                                                       ^^^^^^^^^

@MarshallOfSound
Copy link
Member

At first glance this would appear to cause issues on Windows with node-gyp that very specifically links to node.lib.

Do we have any validation that native modules still work against a renamed node binary with this change?

@joyeecheung
Copy link
Member

@MarshallOfSound test/addons should cover that, I believe.

@joyeecheung
Copy link
Member

I just noticed that we explicitly mentions node.lib in several documentation e.g. it's part of process.release.libUrl

@refack
Copy link
Contributor Author

refack commented Apr 10, 2019

Do we have any validation that native modules still work against a renamed node binary with this change?

Each one of our CI jobs builds several native addons (with node-gyp). But it does seem like there's a bug in the CI script...

This will not land until I'm sure that is resolved.

I just noticed that we explicitly mentions node.lib in several documentation e.g. it's part of process.release.libUrl

I'll grep for these.

@refack
Copy link
Contributor Author

refack commented Apr 11, 2019

Do we have any validation that native modules still work against a renamed node binary with this change?

Each one of our CI jobs builds several native addons (with node-gyp). But it does seem like there's a bug in the CI script...

This will not land until I'm sure that is resolved.

I just noticed that we explicitly mentions node.lib in several documentation e.g. it's part of process.release.libUrl

I'll grep for these.

Ok, so even I got confused. node.lib is an artifact of building the main exe:

     Creating library out\Debug\node.lib and object out\Debug\node.exp
  node.vcxproj -> out\Debug\\node.exe
FinalizeBuildStatus:
  Deleting file "out\Debug\obj\node\node.tlog\unsuccessfulbuild".
  Touching "out\Debug\obj\node\node.tlog\node.lastbuildstate".
Done Building Project "D:\code\node\node.vcxproj" (Build target(s)).

nodelib.lib as a static library is just a partial part of the build, and it could be configured to be a dynamic library in special build configuration.

tl;dr this is not semver major even WRT to downstream embedders.

@nodejs-github-bot
Copy link
Collaborator

@refack
Copy link
Contributor Author

refack commented Apr 11, 2019

/CC @nodejs/build-files @nodejs/platform-windows PTAL

eliminate the need for `rename_node_bin_win`

PR-URL: nodejs#27150
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants