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

Merge v8 changes #1506

Closed
wants to merge 6 commits into from
Closed

Merge v8 changes #1506

wants to merge 6 commits into from

Conversation

chrisdickinson
Copy link
Contributor

This cherry-picks the commits from next to master.

The cherry-picked commits are:

767a5c6
41c00a2
336dc08
bb97b70

There was one conflict involving deps/v8/AUTHORS and deps/v8/v8_version.h. I changed both to match next.

@chrisdickinson chrisdickinson added this to the 2.0.0 milestone Apr 22, 2015
@chrisdickinson chrisdickinson added the v8 engine Issues and PRs related to the V8 dependency. label Apr 22, 2015
@chrisdickinson
Copy link
Contributor Author

R=@domenic, @bnoordhuis, @indutny

@domenic
Copy link
Contributor

domenic commented Apr 22, 2015

Stable is on .14; we should not be on .15

@bnoordhuis
Copy link
Member

4.2.77.15 was the HEAD of branch-heads/4.2 at the time the original PR was merged into next. We're two commits behind, by the way; it's at .17 now.

@domenic
Copy link
Contributor

domenic commented Apr 22, 2015

I'm sorry; I totally forgot my own instructions for picking which tag to use :(. We should probably grab 17 then...

@chrisdickinson
Copy link
Contributor Author

This is probably a silly set of questions, but:

  • Where do we fetch V8 from?
  • Are there any patches (or other, manual actions) we have to apply to add a new V8?

@rvagg
Copy link
Member

rvagg commented Apr 23, 2015

@chrisdickinson when you find answers perhaps you could document somewhere? either the wiki or in docs/

@bnoordhuis
Copy link
Member

@chrisdickinson

$ git clone https://chromium.googlesource.com/v8/v8.git
$ git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
$ cd v8
# if you want to check out a release branch
$ git checkout branch-heads/4.2
$ git` pull --rebase && env PATH=$HOME/src/depot_tools:$PATH ../depot_tools/gclient sync
# to compile a release build
$ make -j8 native  # or x64.release
# to compile debug build without i18n
$ make -j8 x64.debug extrachecks=on i18nsupport=off

@bnoordhuis
Copy link
Member

Are there any patches (or other, manual actions) we have to apply to add a new V8?

Only if there are floating patches. In most cases you can just git log --oneline deps/v8 and cherry-pick them after upgrading.

@chrisdickinson
Copy link
Contributor Author

Hm, I'm running into issues doing this:

  • Git doesn't see any remote (or local) branch-heads/* refs.
  • gclient can't find a .gclient file. I created one using gclient config $(git remote -v | grep fetch | awk '{print $2}') (using the git url as the origin url), but gclient sync seems to just re-clone v8.

@jbergstroem
Copy link
Member

@chrisdickinson I don't think you have to use gclient unless you want to push stuff back. Cloning the git repo, retrieving commits between known states (current tag vs intended next tag) and go through floating patches to see if they've been merged or interferes with applying the new commits could be enough.

@chrisdickinson
Copy link
Contributor Author

@jbergstroem Cool – the other problem is that I don't see any refs (tags or branches) corresponding to branch-heads in my (fresh) git clone.

@ofrobots
Copy link
Contributor

@chrisdickinson to see the branch-heads you need to edit the .git/config file as per instructions from @domenic https://gist.github.com/domenic/aca7774a5d94156bfcc1#checking-out-the-head-of-the-corresponding-branch

@chrisdickinson
Copy link
Contributor Author

@ofrobots Thanks for the link. I'll pull this into a document in /doc/ for folks to reference in the future!

tellnes and others added 4 commits April 24, 2015 12:52
This commit makes `os.tmpdir()` behave consistently on all platforms. It
changes `os.tmpdir()` to always return a path without trailing slash.

Semver: major
Fixes: #715
PR-URL: #747
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This commit applies some secondary changes in order to make `make test`
pass cleanly:

* disable broken postmortem debugging in common.gypi

* drop obsolete strict mode test in parallel/test-repl

* drop obsolete test parallel/test-v8-features

PR-URL: #1232
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Cherry-pick https://codereview.chromium.org/1033733003 from upstream
and re-enable postmortem debugging.

PR-URL: #1232
Reviewed-By: Fedor Indutny <fedor@indutny.com>
This includes the out-of-tree patch (but fixed in upstream HEAD) from
commit 41c00a2 ("deps: enable v8 postmortem debugging again".)

PR-URL: #1399
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@chrisdickinson
Copy link
Contributor Author

To update: I'm seeing errors on tests now that I've merged in V8 4.2.77.18, specifically:

FATAL ERROR: v8::Context::Exit() Cannot exit non-entered context

@chrisdickinson
Copy link
Contributor Author

Ok, tracked down the error. It turns out I hadn't pulled in @indutny's SealHandleScope patch from upstream (I had partially applied enough to get node working, barely.) Updated. PTAL @indutny, @bnoordhuis.

@indutny
Copy link
Member

indutny commented Apr 25, 2015

@chrisdickinson wut? was it fixed by re-introducing SealHandleScope?

@chrisdickinson
Copy link
Contributor Author

@indutny When grabbing the new V8 tree, I checked out branch-heads/4.2, which did not have v8's 1f8555 on them. When I tried to compile, it was missing SealHandleScope. I erroneously reintroduced that class def + implementation manually and recompiled. It seemed to work, until I ran tests at which point the above error started spamming out. Running a debug build did not have the same errors. I looked at a5244d3 and noticed the link to 1f8555. I cherry-picked that commit onto my v8 working tree, then vendored that into iojs and recompiled, and the tests passed. Specifically, I had missed these changes.

@rvagg
Copy link
Member

rvagg commented Apr 27, 2015

let's get this show on the road folks!

@rvagg rvagg mentioned this pull request Apr 27, 2015
@domenic
Copy link
Contributor

domenic commented Apr 27, 2015

Branch head is up to 18, would be good to update this PR to just be a clean upgrade to 18 + any floating patches.

@chrisdickinson
Copy link
Contributor Author

Yep, this is updated to 18.

On Apr 26, 2015, at 9:01 PM, Domenic Denicola notifications@github.com wrote:

Branch head is up to 18, would be good to update this PR to just be a clean upgrade to 18 + any floating patches.


Reply to this email directly or view it on GitHub.

@chrisdickinson
Copy link
Contributor Author

@domenic Ah – did you mean that the upgrade commits should be squashed down to a single commit? I'll do that on merge. Until then I'm leaving the commits I introduced (3d57c7b and a1b16ca) separate so @indutny and @bnoordhuis can review them.

@bnoordhuis
Copy link
Member

LGTM but it would be easier if you split off the out-of-tree patches into separate commits. You seem to be doing that for the postmortem fixes but not SealHandleScope.

@domenic
Copy link
Contributor

domenic commented Apr 27, 2015

Ah the commit log typo is what confused me. Also the additional tmpdir() commit.

@gopat gopat mentioned this pull request Apr 27, 2015
chrisdickinson and others added 2 commits April 27, 2015 11:46
This commit applies a secondary change in order to make `make test`
pass cleanly, specifically re-disabling post-mortem debugging in
common.gypi.
Cherry-pick https://codereview.chromium.org/1033733003 from upstream
and re-enable postmortem debugging.

PR-URL: #1232
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@chrisdickinson
Copy link
Contributor Author

Ah, updated the commit log, sorry about that.

@bnoordhuis: Ah, my goal in partitioning the history they way I did was to make sure that each commit compiled & passed the tests – for the future, I'll leave them separate.

Also, I'm working on documenting the process over here and will PR that soon.

chrisdickinson added a commit that referenced this pull request Apr 28, 2015
This commit applies a secondary change in order to make `make test`
pass cleanly, specifically re-disabling post-mortem debugging in
common.gypi.

PR-URL: #1506
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@chrisdickinson
Copy link
Contributor Author

Merged in 509b59e. Ended up leaving the commits separate.

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

Successfully merging this pull request may close these issues.

8 participants