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

Re-introduce duplicate symbol check #1891

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 26, 2019

Originally in #1689 and was reverted in #1828, we decided in nodejs/node#28647 (comment) it was "breaking" as far as Node was concerned.

Given the dependency chain to get to Node I'm not sure what to do about this. Is it enough to put a semver-major label on it? It's still likely to bubble up to Node 12 (and maybe 10) through their regular npm updates though. Is there a rule in place that would prevent certain versions of npm getting into Node 12? And how do we couple that with our moves to get to Python 3 support and the semver-major nature of them? Do we then prevent Node 12 from getting Python 3 support if we block a semver-major release of node-gyp getting back in to older versions of Node?

/cc @gabrielschulhof @bnoordhuis @nodejs/npm

@rvagg
Copy link
Member Author

rvagg commented Oct 2, 2019

Unless someone thinks this is urgent I think we should punt this till 7.0.0 so we can focus on python 3 compat in 6.x.

@gabrielschulhof
Copy link

@rvagg I have no special reason to push for urgency. It would be good if it landed though, because, as it is, there might be all kinds of weird errors with equally weird solutions.

@cclauss
Copy link
Contributor

cclauss commented Oct 2, 2019

Reading the output was superimpossible because of all the backslashes but at its root the test failure is caused by #1904

@cclauss cclauss mentioned this pull request Oct 2, 2019
2 tasks
Gabriel Schulhof added 2 commits October 3, 2019 11:19
On OSX symbols are exported by default, and they overlap if two
different copies of the same symbol appear in a process. This change
ensures that, on OSX, symbols are hidden by default.

Re: nodejs/node-addon-api#456
Fixes: nodejs/node#26765
PR-URL: #1689
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8
equivalents to avoid OSX test failures with Node.js v12.x.

Thanks @rvagg!

Re: nodejs/node-addon-api#456
Fixes: nodejs/node#26765
PR-URL: #1689
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg rvagg force-pushed the rvagg/reintroduce-duplicate-symbol-check branch from d8e5776 to c26ca1b Compare October 3, 2019 01:19
@rvagg
Copy link
Member Author

rvagg commented Oct 3, 2019

green after rebase to master

@vweevers
Copy link
Contributor

@rvagg I'm wondering why node doesn't use the RTLD_LOCAL flag for dlopen() by default? In my testing (of leveldown on mac) that fixed an overlap of symbols, to the same effect as -fvisibility=hidden.

@rvagg
Copy link
Member Author

rvagg commented Oct 23, 2019

I don't know, historical I suppose, and changing these things is tricky since it has so much breaking potential. But maybe that could be done in a semver-major in Node. @bnoordhuis I'm sure you understand this way better than I, thoughts?

@vweevers
Copy link
Contributor

@rvagg
Copy link
Member Author

rvagg commented Oct 27, 2019

To get some discussion going, maybe try opening a PR against nodejs/node to actually make the change? You'll get a lot of folks coming out of the woodwork with opinions if you did that I reckon.

@vweevers
Copy link
Contributor

vweevers commented Nov 4, 2019

I've surveyed a bunch of native addons and only found real issues in leveldown and rocksdb so far, apart from unrelated issues with shared libraries. So I don't have a strong case for changing node behavior. I'll continue to survey addons, improving my understanding of the issue along the way.

@rvagg
Copy link
Member Author

rvagg commented Jan 6, 2020

sooooo I'm not seeing a path to reintroducing this without coordination between node-gyp, npm and node.js to make sure it lands semver-major in each of them. I just don't see that happening.

@rvagg rvagg closed this Jun 24, 2021
@Trott Trott deleted the rvagg/reintroduce-duplicate-symbol-check branch September 28, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants