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

Update v8_inspector #8150

Closed
wants to merge 2 commits into from
Closed

Conversation

ofrobots
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps, inspector

Description of change

Update to the latest version of upstream v8_inspector.

/cc @eugeneo, @bnoordhuis

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Aug 17, 2016
@bnoordhuis
Copy link
Member

Doesn't seem to build on smartos because the toolchain is so ancient there:

InspectorProtocol.cpp:33:5: error: 'snprintf' is not a member of 'std'
     std::snprintf(buffer, length, "%.3g", number);

The Windows CI failure appears to have been a network outage.

@eugeneo
Copy link
Contributor

eugeneo commented Aug 19, 2016

I added snprintf macro, same as the one in Node sources. Is it possible to
test this on SmartOS - https://github.com/eugeneo/node/tree/snprintf?

The macro is in PlatformSTL.h

On Thu, Aug 18, 2016 at 12:27 AM Ben Noordhuis notifications@github.com
wrote:

Doesn't seem to build on smartos because the toolchain is so ancient there:

InspectorProtocol.cpp:33:5: error: 'snprintf' is not a member of 'std'
std::snprintf(buffer, length, "%.3g", number);

The Windows CI failure appears to have been a network outage.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8150 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrfBfaVrZkD2W3m0I7_i1bY0IHBadks5qhAlsgaJpZM4Jm-sE
.

@bnoordhuis
Copy link
Member

@eugeneo
Copy link
Contributor

eugeneo commented Aug 19, 2016

Thank you. Looks like the approach is correct in principle. I will put the
shim into std:: namespace.

On Fri, Aug 19, 2016 at 11:13 AM Ben Noordhuis notifications@github.com
wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8150 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrUvnWq6XkGplmCGZaUWYzWINkSi5ks5qhfIygaJpZM4Jm-sE
.

@eugeneo
Copy link
Contributor

eugeneo commented Aug 19, 2016

How can I detect SmartOS? From a compiler message, my understanding is that
it has snprintf but not the std::snprintf.

On Fri, Aug 19, 2016 at 2:03 PM Eugene Ostroukhov eostroukhov@google.com
wrote:

Thank you. Looks like the approach is correct in principle. I will put the
shim into std:: namespace.

On Fri, Aug 19, 2016 at 11:13 AM Ben Noordhuis notifications@github.com
wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8150 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrUvnWq6XkGplmCGZaUWYzWINkSi5ks5qhfIygaJpZM4Jm-sE
.

@bnoordhuis
Copy link
Member

In libuv we wrap smartos-specific code in #ifdef _sun guards but that also covers other illumos derivatives and solaris proper. Perhaps @misterdjules has suggestions?

@eugeneo
Copy link
Contributor

eugeneo commented Aug 22, 2016

@bnoordhuis - Can you please test this branch? It imports existing snprintf into the std namespace. Looks like SmartOS has the snprintf in the global namespace.

@bnoordhuis
Copy link
Member

@misterdjules
Copy link

_sun is the right macro to test against when detecting the SmartOS platform.

@eugeneo
Copy link
Contributor

eugeneo commented Aug 23, 2016

I submitted the SmartOS fix to the Chrome yesterday and it is now available in the latest V8 inspector export.

Eugene Ostroukhov added 2 commits August 23, 2016 10:06
Pick up latest from [1] corresponding to the Blink commit 60cd6e859b.

[1]: pavelfeldman/v8_inspector@55f21a5611
@ofrobots
Copy link
Contributor Author

ofrobots commented Aug 23, 2016

Updated the PR to pick up upstream fixes from @eugeneo. New CI: https://ci.nodejs.org/job/node-test-pull-request/3801/

@ofrobots
Copy link
Contributor Author

Launched a new CI: https://ci.nodejs.org/job/node-test-pull-request/3813/ because of hiccups on the last one. AIX seems to be having issues in the CI in general today.

@ofrobots
Copy link
Contributor Author

One more CI: https://ci.nodejs.org/job/node-test-pull-request/3846/. This PR does need an LGTM before I can land this (/cc @bnoordhuis)

@bnoordhuis
Copy link
Member

LGTM

@ofrobots
Copy link
Contributor Author

Landed as 1e0bfac...1b8accf.

@ofrobots ofrobots closed this Aug 30, 2016
ofrobots pushed a commit that referenced this pull request Aug 30, 2016
Pick up latest from [1] corresponding to the Blink commit 60cd6e859b.

[1]: pavelfeldman/v8_inspector@55f21a5611

PR-URL: #8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
ofrobots pushed a commit that referenced this pull request Aug 30, 2016
PR-URL: #8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Pick up latest from [1] corresponding to the Blink commit 60cd6e859b.

[1]: pavelfeldman/v8_inspector@55f21a5611

PR-URL: nodejs#8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
PR-URL: nodejs#8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
Pick up latest from [1] corresponding to the Blink commit 60cd6e859b.

[1]: pavelfeldman/v8_inspector@55f21a5611

PR-URL: #8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
PR-URL: #8150
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@Bilge
Copy link

Bilge commented Dec 27, 2016

I have this problem.

/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp: In function 'void blink::protocol::internal::intToStr(int, char*, size_t)':
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp:23:5: error: 'snprintf' is not a member of 'std'
     std::snprintf(buffer, length, "%d", number);
     ^
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp: In function 'void blink::protocol::internal::doubleToStr(double, char*, size_t)':
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp:28:5: error: 'snprintf' is not a member of 'std'
     std::snprintf(buffer, length, "%f", number);
     ^
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp: In function 'void blink::protocol::internal::doubleToStr3(double, char*, size_t)':
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp:33:5: error: 'snprintf' is not a member of 'std'
     std::snprintf(buffer, length, "%.3g", number);
     ^
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp: In function 'void blink::protocol::internal::doubleToStr6(double, char*, size_t)':
/root/buildroot-2016.11/output/build/nodejs-6.7.0/out/Release/obj/gen/blink/platform/inspector_protocol/InspectorProtocol.cpp:38:5: error: 'snprintf' is not a member of 'std'
     std::snprintf(buffer, length, "%.6g", number);
     ^

I am using a uClibc 0.9.33.2 toolchain (latest) available from this crosstool-NG project.

It is probably incorrect to change compilation strategy based on platform flags instead of detecting feature availability.

@eugeneo
Copy link
Contributor

eugeneo commented Dec 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants