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

test: check symbols in shared lib #18806

Closed
wants to merge 1 commit into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented Feb 15, 2018

When building the node with --shared option, we need
to verify the symbols in shared lib instead of executable.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 15, 2018
// There is no nm in Windows, maybe use dumpbin instead.
// However, we still try to compose the correct shared lib path
if (common.isWindows) {
args.push(path.join(path.dirname(process.execPath), 'node.dll'));
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the logic for getting the shared lib instead of the stub executable is worth putting in the test/common/shared-lib-util.js being added in #18626. The platform specific logic could then be kept out of these changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. that would be great! Since that PR is not landed yet. I will do that after the merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau per your suggestion and the PR is landed, I moved the shared lib path logic into test/common/shared-lib-util.js

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from beedb23 to 39c2edf Compare February 16, 2018 17:35
const { getSharedLibPath } = require('../common/shared-lib-util.js');

// For shared lib case, check shared lib instead
if (process.config.variables.node_shared) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just conditionally create args?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure! :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

it's done.

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 39c2edf to 79842cc Compare February 16, 2018 18:17
@@ -27,3 +27,17 @@ exports.addLibraryPath = function(env) {
(env.PATH ? env.PATH + path.delimiter : '') +
path.dirname(process.execPath);
};

// Get get the full path of shared lib
Copy link
Member

Choose a reason for hiding this comment

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

One too many get's 😁.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops....... thanks @richardlau . fixed

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 79842cc to 0da2151 Compare February 16, 2018 19:39
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018
@BridgeAR
Copy link
Member

@yhwang
Copy link
Member Author

yhwang commented Feb 19, 2018

debian8-x64 failed on async-hooks/test-getaddrinforeqwrap test case. The change only modify test case parallel/test-postmortem-metadata. The failure should be irrelevant. async-hooks/test-getaddrinforeqwrap is not in flaky list. Let me kick off another CI.

https://ci.nodejs.org/job/node-test-commit/16376/

@yhwang
Copy link
Member Author

yhwang commented Feb 19, 2018

previous CI passed the windows build and the second CI passed the linux test. I think it's green for this change.

When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: nodejs#18535

Signed-off-by: Yihong Wang <[email protected]>
@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 0da2151 to 01374fe Compare February 20, 2018 22:36
@yhwang
Copy link
Member Author

yhwang commented Feb 20, 2018

rebased because of the conflict and new CI: https://ci.nodejs.org/job/node-test-commit/16393/

@addaleax
Copy link
Member

Landed in ab7c627

@addaleax addaleax closed this Feb 21, 2018
addaleax pushed a commit that referenced this pull request Feb 21, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@yhwang yhwang deleted the sharedlib-test-fix-postmortem branch February 21, 2018 23:05
@yhwang
Copy link
Member Author

yhwang commented Feb 21, 2018

thanks @addaleax

MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: nodejs#18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: nodejs#18806
Refs: nodejs#18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants