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

deps: hide zlib internal symbols #11082

Closed
wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Jan 31, 2017

Use HAVE_HIDDEN when compiling zlib so it's internal symbols
have __attribute__((visibility ("hidden"))).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Jan 31, 2017
@sam-github
Copy link
Contributor Author

sam-github commented Jan 31, 2017

Running a build to see if all our compilers support gcc-style attributes.

EDIT: https://ci.nodejs.org/job/node-test-pull-request/6124/

@sam-github
Copy link
Contributor Author

I've heard

There's an issue with the latest npm update that breaks windows

So will have to wait a bit and try again.

Use HAVE_HIDDEN when compiling zlib so it's internal symbols
have __attribute__((visibility ("hidden"))).
@sam-github
Copy link
Contributor Author

#11085 (comment) should fix, trying again

@sam-github
Copy link
Contributor Author

@sam-github sam-github added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 31, 2017
@@ -48,6 +48,9 @@
'.',
],
},
'defines': [
'HAVE_HIDDEN',
],
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the 'OS!="win"' conditional below. The __attribute__((visibility("hidden"))) doesn't work with Visual Studio (see https://ci.nodejs.org/job/node-compile-windows/6701/label=win-vs2015/consoleFull) and it probably isn't needed anyway because we use deps/zlib/win32/zlib.def to control what is and isn't exported.

Alternatively, if you teach tools/mkssldef.py to generate UNIX linker maps, we can unify the two approaches.

@sam-github
Copy link
Contributor Author

@sam-github sam-github mentioned this pull request Feb 1, 2017
2 tasks
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

Both gcc and clang support this. CI is green.

jasnell pushed a commit that referenced this pull request Feb 2, 2017
Use HAVE_HIDDEN when compiling zlib so it's internal symbols
have __attribute__((visibility ("hidden"))).

PR-URL: #11082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

Landed in c9e5178

@jasnell jasnell closed this Feb 2, 2017
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Use HAVE_HIDDEN when compiling zlib so it's internal symbols
have __attribute__((visibility ("hidden"))).

PR-URL: nodejs#11082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@jasnell jasnell mentioned this pull request Apr 4, 2017
@sam-github sam-github deleted the zlib-use-hidden branch April 17, 2017 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants