Skip to content

build,win: rename node.lib to libnode.lib#27150

Merged
refack merged 1 commit intonodejs:masterfrom
refack:gyp-rename-libnode
Apr 19, 2019
Merged

build,win: rename node.lib to libnode.lib#27150
refack merged 1 commit intonodejs:masterfrom
refack:gyp-rename-libnode

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 commented Apr 9, 2019

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3210

@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 commented Apr 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22294/

@refack refack mentioned this pull request Apr 9, 2019
Closed
3 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22298/

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

nodejs-github-bot commented Apr 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22309/

@refack refack added the review wanted PRs that need reviews. label Apr 9, 2019
joyeecheung
joyeecheung approved these changes Apr 9, 2019
View reviewed changes
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.

node.gyp Outdated
Copy link
Member

@joyeecheung joyeecheung Apr 9, 2019

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

@refack refack Apr 10, 2019

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

MarshallOfSound commented Apr 10, 2019

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

joyeecheung commented Apr 10, 2019

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

@joyeecheung
Copy link
Member

joyeecheung commented Apr 10, 2019

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

nodejs-github-bot commented Apr 11, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22361/

@refack
Copy link
Contributor Author

refack commented Apr 11, 2019

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

@refack
build,win: rename node.lib to libnode.lib
88beaf0 
eliminate the need for `rename_node_bin_win`

PR-URL: nodejs#27150
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack refack force-pushed the gyp-rename-libnode branch from ef7e55a to 88beaf0 Compare April 19, 2019 18:19
@refack refack merged commit 88beaf0 into nodejs:master Apr 19, 2019
@refack refack removed the review wanted PRs that need reviews. label Apr 19, 2019
@refack refack removed their assignment Apr 19, 2019
@refack refack deleted the gyp-rename-libnode branch April 19, 2019 22:08
@refack refack assigned refack and unassigned refack Apr 19, 2019
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
This was referenced Jan 12, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Feb 9, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@kissofire kissofire mentioned this pull request May 22, 2024
Open
@137717unity 137717unity mentioned this pull request May 22, 2024
Open
@wedataintelligence wedataintelligence mentioned this pull request May 22, 2024
Open
@baby636 baby636 mentioned this pull request May 22, 2024
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request May 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@refack @nodejs-github-bot @MarshallOfSound @joyeecheung