-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: add maintaining info for shared library option #42517
Conversation
Refs: nodejs#41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in nodejs#41850 Signed-off-by: Michael Dawson <mdawson@devrus.com>
Review requested:
|
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.
lgtm
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@mscdex thanks for the review, can't believe I spelled library wrong so many times. |
Co-authored-by: Tierney Cyren <accounts@bnb.im>
Co-authored-by: Tierney Cyren <accounts@bnb.im>
@bnb comments addressed. |
For the node.exe wrapper, it is built so that it can | ||
find the shared library in one of the following: | ||
|
||
* the same directory as the node executable | ||
* ../lib with the expectation that the executable is | ||
installed in a `bin` directory at the same level | ||
as a `lib` directory in which the shared library is | ||
installed. This is where the default package that | ||
is build with the shared library option will | ||
place the executable and library. |
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.
I think this is only true on Linux and macOS where we can embed a relative RPATH into the executable. AIX doesn't have this luxury so currently it hard codes an absolute path to libnode.a into the executable.
On Windows libnode.dll and node.exe sit in the same directory as this is the expected location for DLLs. See this MSDN article if you want to know more about the DLL search order on Windows.
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.
@lux01 updated, can you tell me what the hardcoded path for AIX is so that I can add it?
@lux01 I think this now reflects the current implementation in terms of what was landed. Can you take one more look before I land it? |
Signed-off-by: Michael Dawson <mdawson@devrus.com>
I'm going to go ahead and land and I'll update if there are any follow on comments. |
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Landed in 8b415e8 |
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in #41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: nodejs/node#41850 I think it would be good to have some info/context for maintainers on the shared library option. Add that based on disussion in nodejs/node#41850 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs/node#42517 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #41850
I think it would be good to have some info/context
for maintainers on the shared library option. Add that
based on disussion in #41850
Signed-off-by: Michael Dawson mdawson@devrus.com