Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 2321433003: Fix build on Solaris and musl-based platforms (Closed)

Created:
4 years, 3 months ago by targos
Modified:
4 years, 3 months ago
Reviewers:
ofrobots, rmcilroy
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Fix build on Solaris and musl-based platforms Use a similar approach to Chromium [1] to allow platforms that don't have execinfo.h to build. Because Solaris has it, add this platform to the condition. [1] https://cs.chromium.org/chromium/src/base/debug/stack_trace_posix.cc BUG= R=rmcilroy@chromium.org

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -3 lines) Patch
M src/base/debug/stack_trace.cc View 1 chunk +2 lines, -0 lines 2 comments Download
M src/base/debug/stack_trace_posix.cc View 6 chunks +13 lines, -3 lines 1 comment Download

Messages

Total messages: 8 (1 generated)
targos
4 years, 3 months ago (2016-09-07 08:13:58 UTC) #2
rmcilroy
Thanks for the CL, a couple of comments. https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace.cc File src/base/debug/stack_trace.cc (right): https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace.cc#newcode34 src/base/debug/stack_trace.cc:34: #if ...
4 years, 3 months ago (2016-09-07 11:06:22 UTC) #3
targos
On 2016/09/07 at 11:06:22, rmcilroy wrote: > Thanks for the CL, a couple of comments. ...
4 years, 3 months ago (2016-09-12 07:33:12 UTC) #4
targos
https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace.cc File src/base/debug/stack_trace.cc (right): https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace.cc#newcode34 src/base/debug/stack_trace.cc:34: #if V8_LIBC_GLIBC || V8_OS_BSD || V8_OS_SOLARIS On 2016/09/07 at ...
4 years, 3 months ago (2016-09-12 07:34:54 UTC) #5
rmcilroy
Could you split this up please. Once CL for Solaris and one for musl. https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace.cc#newcode34 ...
4 years, 3 months ago (2016-09-12 07:51:59 UTC) #6
ofrobots
On 2016/09/12 07:51:59, rmcilroy wrote: > Could you split this up please. Once CL for ...
4 years, 3 months ago (2016-09-12 12:41:34 UTC) #7
ofrobots
4 years, 3 months ago (2016-09-12 21:59:31 UTC) #8
On 2016/09/12 12:41:34, ofrobots wrote:
> On 2016/09/12 07:51:59, rmcilroy wrote:
> > Could you split this up please. Once CL for Solaris and one for musl.
> > 
> >
>
https://codereview.chromium.org/2321433003/diff/1/src/base/debug/stack_trace....
> > > src/base/debug/stack_trace.cc:34: #if V8_LIBC_GLIBC || V8_OS_BSD ||
> > > V8_OS_SOLARIS
> > > On 2016/09/07 at 11:06:22, rmcilroy wrote:
> > > > This will avoid printing on windows also which is wrong. The
corresponding
> > > change in Chromium is "#if !defined(__UCLIBC__)". Do you even need this
> though
> > > (the CL mentions Solaris, and this change allows suggests Solaris can call
> > > OutputToStream anyway.
> > > 
> > > Would adding "|| V8_OS_WIN" be enough for Windows ?
> > 
> > I would rather you found a way to explicitly exclude musl here (like
chromium
> > does for ulibc) rather than whitelisting platforms.
> 
> @targos: Here's my version of your change that does this:
>
https://github.com/ofrobots/node/commit/5a70fadee25d67d9e3997f33e25ea8432b701a7b.

Superceded by https://codereview.chromium.org/2333023002

Powered by Google App Engine
This is Rietveld 408576698