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

doc: add maintaining info for shared library option #42517

Closed
wants to merge 26 commits into from

Conversation

mhdawson
Copy link
Member

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

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>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 29, 2022
@mscdex mscdex changed the title doc: add maintaining info for shared libary option doc: add maintaining info for shared library option Mar 29, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

mhdawson and others added 11 commits March 30, 2022 09:20
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>
@mhdawson
Copy link
Member Author

@mscdex thanks for the review, can't believe I spelled library wrong so many times.

@mhdawson
Copy link
Member Author

mhdawson commented Apr 7, 2022

@bnb comments addressed.

doc/contributing/maintaining-shared-library-support.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-shared-library-support.md Outdated Show resolved Hide resolved
Comment on lines 52 to 61
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.
Copy link
Contributor

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.

Copy link
Member Author

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?

doc/contributing/maintaining-shared-library-support.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

@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?

@mhdawson
Copy link
Member Author

I'm going to go ahead and land and I'll update if there are any follow on comments.

mhdawson added a commit that referenced this pull request May 12, 2022
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>
@mhdawson
Copy link
Member Author

Landed in 8b415e8

@mhdawson mhdawson closed this May 12, 2022
BethGriggs pushed a commit that referenced this pull request May 16, 2022
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>
@RafaelGSS RafaelGSS mentioned this pull request May 16, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
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>
targos pushed a commit that referenced this pull request Jul 12, 2022
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>
targos pushed a commit that referenced this pull request Jul 31, 2022
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>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants