test: add lib path env when node_shared=true#18626
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the build 
Makefile Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
it's only for macOS. so you mean make it no-op for other platform?
Makefile Outdated
There was a problem hiding this comment.
Is /usr/local/lib hardcoded somewhere else, or should we use the actual prefix here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@yhwang I guess it’s coming from $(PREFIX) or ./configure --prefix=…?
There was a problem hiding this comment.
Oh, I see. if that's the case, let me change it to variable. Thanks!
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@addaleax I updated my change to use @rpath. Then I reverted my change in Makefile. You may take a look.
Makefile Outdated
test/common/index.js Outdated
There was a problem hiding this comment.
Should this extend LD_LIBRARY_PATH instead of replace it? (Same for LIBPATH and DYLD_LIBRARY_PATH below).
There was a problem hiding this comment.
Sure. Let me change to extend these variables.
yhwangforce-pushed the sharedlib-fix-testcase-lib-not-found branch 2 times, most recently from b354c8c to 0295775Compare
yhwangforce-pushed the sharedlib-fix-testcase-lib-not-found branch 2 times, most recently from 7318472 to eed929eCompare
BridgeAR commented Feb 10, 2018
BridgeAR added the author ready 
yhwang commented Feb 12, 2018
failed on a flaky test case: |
test/common/index.js Outdated
There was a problem hiding this comment.
This is currently not used at all, right? If so, I would rather remove it again.
test/common/index.js Outdated
There was a problem hiding this comment.
Since this is only going to be called rarely, I feel like it would be better to move this to a separate file.
There was a problem hiding this comment.
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 was a problem hiding this comment.
There is for example test/common/countdown.js. Similar to that you can just add another file.
There was a problem hiding this comment.
@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]>
yhwangforce-pushed the sharedlib-fix-testcase-lib-not-found branch from eed929e to b6f1ae3Compareyhwang commented Feb 13, 2018
May I have another CI to verify this PR again after I refactor the function to a separated js file? |
mhdawson commented Feb 15, 2018
yhwang commented Feb 15, 2018
Only failed on pi1. However, on those 6 runs on pi1. 2 runs passed! (If I interpret the results correctly.) |
BridgeAR commented Feb 16, 2018
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: #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]>
Should this be backported to 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 commented Aug 7, 2018
@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]>
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: #18626 Refs: #18535 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
7 participants
When building the node with
--sharedoption, the major output is theshared 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,
-brtllinker 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), orvcbuild test(Windows) passesAffected core subsystem(s)
test, build