test: add lib path env when node_shared=true #18626

yhwang · 2018-02-07T18:41:51Z

When building the node with --shared option, the major output is the
shared library. However, we still build a node executable which links
to the shared lib. It's for testing purpose. When testing with the
executable, some test cases move/copy the executable, change the relative
path to the shared library and fail. Using lib path env would solve the
issue. However, in macOS, need to modify the dependency in the
executable by using the install_name_tool utility. In AIX, -brtl
linker option rebinds the symbols in the executable and addon modules
could use them.

Refs: #18535

Signed-off-by: Yihong Wang [email protected]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, build

addaleax · 2018-02-08T00:21:56Z

Makefile

@@ -239,6 +239,9 @@ v8:
 .PHONY: test
 # This does not run tests of third-party libraries inside deps.
 test: all ## Runs default tests, linters, and builds docs.
+ifeq ($(OSTYPE)$(NODE_TARGET_TYPE),darwinshared_library)
+	$(MAKE) -s change-install-name
+endif


Maybe it would be better to make change-install-name a no-op itself, rather than making its usage conditional? The current implementation wouldn’t work on other OSes anyway, right?

it's only for macOS. so you mean make it no-op for other platform?

@yhwang Yes, exactly that 👍

addaleax · 2018-02-08T00:22:15Z

Makefile

+# to the shared library with abs path.
+.PHONY: change-install-name
+change-install-name:
+	install_name_tool -change /usr/local/lib/libnode.$(NODE_MODULE_VERSION).dylib \


Is /usr/local/lib hardcoded somewhere else, or should we use the actual prefix here?

It's a good question. I don't know where it comes from. and that's important for shared lib embedders. If they build the shared lib, they may want to change it. And that's for macOS only.

@yhwang I guess it’s coming from $(PREFIX) or ./configure --prefix=…?

Oh, I see. if that's the case, let me change it to variable. Thanks!

@addaleax oops, seems the /usr/local/lib comes from here: https://github.com/nodejs/node/blob/master/tools/gyp/pylib/gyp/xcode_emulation.py#L763
And it doesn't honor PREFIX..... is this a bug?

In Linux we use $ORIGIN in rpath when linking. For macOS, we can use @loader_path. Should I add a TODO in the comment and do the change in other PR?
In here, I will leave it to /usr/local/lib for now. is that okay?

[Edit] There is @rpath since macOS 10.5

@addaleax I updated my change to use @rpath. Then I reverted my change in Makefile. You may take a look.

addaleax · 2018-02-08T00:22:28Z

Makefile

+.PHONY: change-install-name
+change-install-name:
+	install_name_tool -change /usr/local/lib/libnode.$(NODE_MODULE_VERSION).dylib \
+	   $(shell pwd)/out/Release/libnode.$(NODE_MODULE_VERSION).dylib \


Release → BUILDTYPE?

Good catch. Let me work on it.

richardlau · 2018-02-08T16:35:35Z

test/common/index.js

+    return;
+  }
+  process.env.LD_LIBRARY_PATH =
+    path.join(path.dirname(process.execPath), 'lib.target');


Should this extend LD_LIBRARY_PATH instead of replace it? (Same for LIBPATH and DYLD_LIBRARY_PATH below).

Sure. Let me change to extend these variables.

BridgeAR · 2018-02-10T19:40:32Z

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

yhwang · 2018-02-12T17:29:33Z

failed on a flaky test case: test_callback_scope/test-resolve-async. It means the CI should be good for this PR.

BridgeAR · 2018-02-12T17:32:36Z

test/common/index.js

@@ -802,3 +802,32 @@ exports.hijackStdout = hijackStdWritable.bind(null, 'stdout');
 exports.hijackStderr = hijackStdWritable.bind(null, 'stderr');
 exports.restoreStdout = restoreWritable.bind(null, 'stdout');
 exports.restoreStderr = restoreWritable.bind(null, 'stderr');
+
+// Check if the executable links to the shared lib node or not.
+exports.isSharedLibNode = process.config.variables.node_shared === true;


This is currently not used at all, right? If so, I would rather remove it again.

BridgeAR · 2018-02-12T17:34:10Z

test/common/index.js

+  process.env.PATH =
+    (process.env.PATH ? process.env.PATH + path.delimiter : '') +
+    path.dirname(process.execPath);
+};


Since this is only going to be called rarely, I feel like it would be better to move this to a separate file.

move to a file which both test-child-process-fork-exec-path and test-module-loading-globalpaths can access? not familiar with the directory structure of the testing suites, any suggestion?

There is for example test/common/countdown.js. Similar to that you can just add another file.

@BridgeAR I moved the function to a new file: shared-lib-util.js

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]>

BridgeAR

LGTM

yhwang · 2018-02-13T20:37:30Z

May I have another CI to verify this PR again after I refactor the function to a separated js file?

mhdawson · 2018-02-15T20:32:11Z

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

yhwang · 2018-02-15T21:45:20Z

Only failed on pi1. However, on those 6 runs on pi1. 2 runs passed! (If I interpret the results correctly.)

BridgeAR · 2018-02-16T16:32:25Z

Landed in ec9e792 🎉

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]> PR-URL: nodejs#18626 Refs: nodejs#18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]> PR-URL: #18626 Refs: #18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]> PR-URL: nodejs#18626 Refs: nodejs#18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-08-07T17:49:34Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

edit: got it to land

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]> PR-URL: #18626 Refs: #18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

yhwang · 2018-08-07T18:31:53Z

@MylesBorins thanks.

When building the node with `--shared` option, the major output is the shared library. However, we still build a node executable which links to the shared lib. It's for testing purpose. When testing with the executable, some test cases move/copy the executable, change the relative path to the shared library and fail. Using lib path env would solve the issue. However, in macOS, need to change the install name for the shared library and use rpath in the executable. In AIX, `-brtl` linker option rebinds the symbols in the executable and addon modules could use them. Signed-off-by: Yihong Wang <[email protected]> PR-URL: #18626 Refs: #18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 7, 2018

addaleax reviewed Feb 8, 2018

View reviewed changes

richardlau reviewed Feb 8, 2018

View reviewed changes

yhwang force-pushed the sharedlib-fix-testcase-lib-not-found branch 2 times, most recently from b354c8c to 0295775 Compare February 8, 2018 20:06

richardlau approved these changes Feb 8, 2018

View reviewed changes

yhwang force-pushed the sharedlib-fix-testcase-lib-not-found branch 2 times, most recently from 7318472 to eed929e Compare February 8, 2018 21:46

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018

BridgeAR reviewed Feb 12, 2018

View reviewed changes

yhwang force-pushed the sharedlib-fix-testcase-lib-not-found branch from eed929e to b6f1ae3 Compare February 13, 2018 00:32

BridgeAR approved these changes Feb 13, 2018

View reviewed changes

richardlau mentioned this pull request Feb 16, 2018

test: check symbols in shared lib #18806

Closed

2 tasks

BridgeAR closed this Feb 16, 2018

yhwang deleted the sharedlib-fix-testcase-lib-not-found branch February 16, 2018 17:14

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added backport-requested-v8.x labels Aug 7, 2018

MylesBorins removed the backport-requested-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add lib path env when node_shared=true #18626

test: add lib path env when node_shared=true #18626

yhwang commented Feb 7, 2018

addaleax Feb 8, 2018

yhwang Feb 8, 2018

addaleax Feb 8, 2018

addaleax Feb 8, 2018

yhwang Feb 8, 2018

addaleax Feb 8, 2018

yhwang Feb 8, 2018

yhwang Feb 8, 2018

yhwang Feb 8, 2018 •

edited

Loading

yhwang Feb 8, 2018

addaleax Feb 8, 2018

yhwang Feb 8, 2018

richardlau Feb 8, 2018

yhwang Feb 8, 2018

BridgeAR commented Feb 10, 2018

yhwang commented Feb 12, 2018

BridgeAR Feb 12, 2018

yhwang Feb 12, 2018

BridgeAR Feb 12, 2018

yhwang Feb 12, 2018

BridgeAR Feb 12, 2018

yhwang Feb 13, 2018

BridgeAR left a comment

yhwang commented Feb 13, 2018

mhdawson commented Feb 15, 2018

yhwang commented Feb 15, 2018

BridgeAR commented Feb 16, 2018

MylesBorins commented Aug 7, 2018 •

edited

Loading

yhwang commented Aug 7, 2018

test: add lib path env when node_shared=true #18626

test: add lib path env when node_shared=true #18626

Conversation

yhwang commented Feb 7, 2018

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 10, 2018

yhwang commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

yhwang commented Feb 13, 2018

mhdawson commented Feb 15, 2018

yhwang commented Feb 15, 2018

BridgeAR commented Feb 16, 2018

MylesBorins commented Aug 7, 2018 • edited Loading

yhwang commented Aug 7, 2018

yhwang Feb 8, 2018 •

edited

Loading

MylesBorins commented Aug 7, 2018 •

edited

Loading