v8: add missing ',' in OpenBSD's 'sources' section by qbit · Pull Request #18448 · nodejs/node · GitHub

Checklist

make (OpenBSD)

Affected core subsystem(s)

v8

I can run a diff upstream as well - but IMO it would be better if someone like @bnoordhuis did, as I don't know who does the reviews in v8 :D

I can run a diff upstream as well - but IMO it would be better if someone like @bnoordhuis did, as I don't know who does the reviews in v8 :D

Fwiw, the guide at https://github.com/v8/v8/wiki/Contributing has become pretty usable.

I’m not sure whether this can actually land upstream though, because they are migrating (have migrated?) away from gyp anyway and the file is not in upstream src/ … @hashseed what should we do here?

Nice! The file is still in the tree : https://github.com/v8/v8/blob/master/gypfiles/v8.gyp#L2066

Don't expect gyp files to make it into the next V8 release. That being said, I would like to ensure that gyp files are in a good state before removing them from V8 and handing them over to Node.js. Since the file moved, floating this patch would be somewhat annoying.

I took the liberty of fixing this upstream.

Crud - I spoke too soon (committed too soon? :P)! Missed this in my diff:

diff --git a/deps/v8/src/v8.gyp b/deps/v8/src/v8.gyp
index 769078479e..a7ce858022 100644
--- a/deps/v8/src/v8.gyp
+++ b/deps/v8/src/v8.gyp
@@ -2068,6 +2068,7 @@
                 '-L/usr/local/lib -lexecinfo',
             ]},
             'sources': [
+              'base/debug/stack_trace_posix.cc',
               'base/platform/platform-openbsd.cc',
               'base/platform/platform-posix.h',
               'base/platform/platform-posix.cc',

I will re-run my :test before pushing a fix - can we hold of on merging this for a little bit?

Updated the upstream patch.

stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources

OK, updated the pr to include stack_trace_posix.cc. I also added another commit to add a -I to cflags in common.gyp.. I can remove that and make another PR if needed.

Upstream patch also merged.

CI https://ci.nodejs.org/job/node-test-pull-request/12885/

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in eb82b21 and 68d9f63

Thanks so much for addressing this, guys! ❤️ Built like a charm on OpenBSD 6.1.

@hashseed I'm attempting to compile a standalone binary of V8 itself (mainly for debugging and examining disassembler output), but the depot_tools raises an error during setup:

Traceback (most recent call last):
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 2857, in <module>
		sys.exit(main(sys.argv[1:]))
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 2843, in main
		return dispatcher.execute(OptionParser(), argv)
	File "/home/Alhadis/Mirrors/depot_tools/subcommand.py", line 252, in execute
		return command(parser, args[1:])
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 2598, in CMDsync
		ret = client.RunOnDeps('update', args)
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 1584, in RunOnDeps
		work_queue.flush(revision_overrides, command, args, options=self._options)
	File "/home/Alhadis/Mirrors/depot_tools/gclient_utils.py", line 1075, in run
		self.item.run(*self.args, **self.kwargs)
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 954, in run
		self.ParseDepsFile()
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 772, in ParseDepsFile
		for key, value in self.parent.get_vars().iteritems():
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 1277, in get_vars
		'host_os': _detect_host_os(),
	File "/home/Alhadis/Mirrors/depot_tools/gclient.py", line 1295, in _detect_host_os
		return _PLATFORM_MAPPING[sys.platform]
KeyError: 'openbsd6'

Line 1295 in depot_tools/gclient.py points to this:

_PLATFORM_MAPPING = {
  'cygwin': 'win',
  'darwin': 'mac',
  'linux2': 'linux',
  'win32': 'win',
  'aix6': 'aix',
}

Tampering with the platform mapping by adding 'openbsd6': 'bsd' in there only raised more problems with getting the build to start...

Could this be amended to accommodate OpenBSD users? Or is it possible to build V8 without the depot_tools? V8's wiki-page on "Building from Source" makes that sound like a bad idea:

Don’t simply git clone the V8 repository!

There are a few examples of building t stand-alone via the ports tree (lang/libv8 - and iirc I sent a few updated versions). The current stand-alone version is fairly old, but with the patches in that or, you should be able to build more recent versions (without depottools)

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes nodejs#15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: nodejs#18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This landed cleanly on v8.x. Opted to not land on v6.x, let me know if that was incorrect

That's fine, OpenBSD is on the v8.x line now.. and v6 wasn't impacted.

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes nodejs#15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: nodejs#18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add stack_trace_posix to 'sources' as well. Fixes nodejs#15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources PR-URL: nodejs#18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jan 30, 2018

danbev approved these changes Jan 30, 2018

View reviewed changes

qbit added 2 commits January 30, 2018 15:12

v8: add missing ',' in OpenBSD's 'sources' section. Add

09134e5

stack_trace_posix to 'sources' as well. Fixes #15784 Fixes nodejs/help#992 add stack_trace_posix to OpenBSD's sources

build: add cflags for OpenBSD, remove stray comma.

f55b677

jasnell approved these changes Feb 2, 2018

View reviewed changes

jasnell pushed a commit that referenced this pull request Feb 2, 2018

build: add cflags for OpenBSD, remove stray comma.

68d9f63

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell closed this Feb 2, 2018

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

build: add cflags for OpenBSD, remove stray comma.

680ca8a

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

build: add cflags for OpenBSD, remove stray comma.

712084a

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

build: add cflags for OpenBSD, remove stray comma.

9c6bb5f

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

build: add cflags for OpenBSD, remove stray comma.

711bafe

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added dont-land-on-v6.x labels Mar 20, 2018

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

build: add cflags for OpenBSD, remove stray comma.

954ef17

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 28, 2018

build: add cflags for OpenBSD, remove stray comma.

d594a68

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 30, 2018

build: add cflags for OpenBSD, remove stray comma.

424703a

PR-URL: #18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

build: add cflags for OpenBSD, remove stray comma.

182f703

PR-URL: nodejs#18448 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

v8: add missing ',' in OpenBSD's 'sources' section by qbit · Pull Request #18448 · nodejs/node · GitHub

Uh oh!

Uh oh!

v8: add missing ',' in OpenBSD's 'sources' section - #18448

v8: add missing ',' in OpenBSD's 'sources' section#18448qbit wants to merge 2 commits intonodejs:masternodejs/node:masterfrom qbit:openbsd_buildCopy head branch name to clipboard

Conversation

qbit commented Jan 30, 2018

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Jan 30, 2018

Uh oh!

qbit commented Jan 30, 2018

Uh oh!

hashseed commented Jan 30, 2018

Uh oh!

qbit commented Jan 30, 2018

Uh oh!

hashseed commented Jan 30, 2018

Uh oh!

qbit commented Jan 30, 2018

Uh oh!

hashseed commented Feb 1, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

jasnell commented Feb 2, 2018

Uh oh!

Alhadis commented Feb 2, 2018

Uh oh!

qbit commented Feb 2, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

qbit commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

v8: add missing ',' in OpenBSD's 'sources' section#18448
qbit wants to merge 2 commits into
nodejs:masterfrom
qbit:openbsd_build

qbit commented Mar 20, 2018 •
edited
Loading