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

Build, warning, header, and include fixes #10139

Closed
wants to merge 15 commits into from

Conversation

brodycj
Copy link

@brodycj brodycj commented Dec 6, 2016

Checklist
  • make -j8 test (UNIX [macOS]; Linux i386 & amd64) and vcbuild test nosign (.\vcbuild.bat nosign then .\vcbuild.bat test nosign nobuild as Administrator on Windows) PASS OK
  • commit message follows commit guidelines
Additional item
Affected core subsystem(s)

build, crypto, dtrace, url, tls, other src

Description of change

General:

  • Resolve warnings on Windows vcbuild (with noperfctr and new without-inspector/without-profiler options), Ubuntu Linux (i386/amd64), and macOS
  • Cleanup node.h
  • Other cleanup: remove NODE_WANT_INTERNALS and extra includes

This PR includes the changes I originally raised in #9757 & #9767.

Details of proposed changes:

  • --without-profiler option to build with code related to v8-profiler.h disabled
  • vcbuild.bat add without-inspector & without-profiler options
  • url, crypto, dtrace, tls, and other src fixes to resolve warnings in src on macOS, Ubuntu Linux (i386), and Windows builds
  • Fix DecodeBytes & DecodeWrite to return portable ptrdiff_t type according to TODO note by @tjfontaine
  • Fix dependencies in node.h: remove conditional include node_internals.h, remove break in node namespace, etc.
  • Remove NODE_WANT_INTERNALS from src and node.gyp files
  • remove extra includes from src files

Other notes:

  • On *nix (macOS/Linux) I seem to get a message that it could not find a "stdin-setrawmode.out" file but the test passes OK.
  • I have to run the tests as Administrator for the symlink tests to pass.
  • .\vcbuild.bat nosign noperfctr without-inspector without-profiler builds with zero warnings in src on Windows with these changes.
  • If I build on macOS with -Wconversion I get a bunch of conversion warnings, even with the changes proposed here.
  • Some changes are marked // TBD POSSIBLE DATA LOSS: since I think we should examine these conversions at some point. I would be happy to take some or all of these out if necessary.

I put all of these changes together since I think they are indirectly related and think they should be kept in the order proposed here. I kept the crypto and tls changes in separately marked commits. I would be happy to split this into multiple PRs as long as they are not merged out of order. I would also be happy to squash some of the commits and rebase if necessary.

TBD items for FUTURE consideration:

  • Makefile remove CPPLINT_EXCLUDE += src/queue.h (no longer there)
  • Include cpplint in Windows test
  • Move src/tree.h to a deps` subdirectory (easier way to exclude it from cpplint)
  • Move src/node_root_certs.h to a special subdirectory
  • Automatically detect the conversion warnings on *nix (and resolve them in src)

Christopher J. Brody added 15 commits December 6, 2016 09:37
Hides ugly warnings in Windows build
Resolve macro redefinition warning on Windows
This is to resolve an unused result warning in node_url.cc.
Resolve warning in Ubuntu Linux i386 build
Resolve conversion warning on Windows
Resolve conversion warning on Windows
Resolve warning on Windows build
Some TBD POSSIBLE DATA LOSS scenarios marked.
* remove node_internals.h dependency from node.h; adjust other source
* remove break in namespace node block from node.h
* move includes to beginning of node.h
* remove extra include from node.h
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Dec 6, 2016
@addaleax
Copy link
Member

addaleax commented Dec 6, 2016

I would be happy to split this into multiple PRs as long as they are not merged out of order.

It would probably help quite a bit with the review process if you split this into independent PRs where that makes sense, yes

@brodycj
Copy link
Author

brodycj commented Dec 6, 2016

Thanks @addaleax, my idea is to split this into 2 parts:

  1. build/warning fixes
  2. header fixes

In case part 1 becomes too big I can always split it again.

I would also like to wait for part 1 to be reviewed and landed before submitting part 2.

Closing for now, please respond if I should do this a different way.

@brodycj
Copy link
Author

brodycj commented Dec 6, 2016

I lied. I just raised #10140, #10141, #10142, #10143, and #10145 to contribute the build and warning fixes in smaller pieces. I hope to see these reviewed soon. Once we deal with these changes then I would be happy to submit the header and include fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants