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

deps: cherry-pick d721121 from v8 upstream #7632

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 9, 2016

Original commit message:

Quit creating array literal boilerplates from Crankshaft.

It's such a corner case.

BUG=

Review URL: https://codereview.chromium.org/1865013002

Cr-Commit-Position: refs/heads/master@{#35346}

Fixes: #7454

R=@nodejs/v8

CI: https://ci.nodejs.org/job/node-test-pull-request/3230/
CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/187/

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Jul 9, 2016
@targos
Copy link
Member

targos commented Jul 9, 2016

LGTM

Original commit message:

    Quit creating array literal boilerplates from Crankshaft.

    It's such a corner case.

    BUG=

    Review URL: https://codereview.chromium.org/1865013002

    Cr-Commit-Position: refs/heads/master@{nodejs#35346}

Fixes: nodejs#7454
PR-URL: nodejs#7632
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@bnoordhuis
Copy link
Member Author

Green apart from a flaky network test. Landed in 839f3d9, thanks for the review.

@bnoordhuis bnoordhuis merged commit 839f3d9 into nodejs:master Jul 11, 2016
@bnoordhuis
Copy link
Member Author

Crap, I forgot to bump the patchlevel. Mea culpa, I'll follow up with a PR later today.

@ofrobots
Copy link
Contributor

We should get confirmation from the V8 team that it is okay for us to be bumping the patch level for the 5.1 branch for our copy given that it is freshly abandoned. If a merge happens upstream, we would get into a very confusing state with the version numbers. /cc @natorion.

@targos
Copy link
Member

targos commented Jul 11, 2016

Is it a new rule that we have to bump the patch level ?

@bnoordhuis
Copy link
Member Author

We should start doing that once we become the de facto upstream, yes. My merge request for 5.1 got rejected earlier today so I think we've reached that point. :-)

@MylesBorins
Copy link
Contributor

@bnoordhuis we are having an LTS meeting Monday morning with stakeholders from V8 to discuss having official LTS release streams so that we can use their official process to backport changes.

Would you like to join?

--> nodejs/Release#119

@bnoordhuis
Copy link
Member Author

s/Monday/Wednesday/? It says the date is the 13th. I would but I don't know if I can.

@MylesBorins
Copy link
Contributor

@bnoordhuis it is indeed Wednesday... we were picking between dates and I had too many in my head. If you can't make it please feel free to let me know anything you think should be mentioned and I'll make sure it is brought up

@bnoordhuis
Copy link
Member Author

I'll see if I can join. In case I can't make it:

  1. If we're going to use Google's infrastructure, what time frame are we looking at? I understand @ofrobots's concern about conflicting version numbers but don't think maintenance should come to a standstill while we wait.
  2. Apropos infrastructure, I understand builders are torn down when a release branch is EOL'd. Does that mean only a subset of the normal waterfall is used and is it still worthwhile for us in that case? We can ramp up our own infrastructure with only modest effort.
  3. Using Google's infrastructure, we'll need to test changes against our test suite separately. Inconvenient, can something be done about that?

@evanlucas
Copy link
Contributor

should this land in v6 or does it need to wait since v6 is still on v8 v5.0.x?

@targos
Copy link
Member

targos commented Jul 15, 2016

This should not land in v6. The v6.x version of this patch is here

@evanlucas
Copy link
Contributor

Ah thanks @targos!

@ofrobots
Copy link
Contributor

@ofrobots's concern about conflicting version numbers but don't think maintenance should come to a standstill while we wait.

My concern was only that we should notify upstream that we have started bumping the patch level. Given that V8 5.1 was so 'fresly abandonded' it is sometimes possible that upstream might change plans and allow a few more merges after one has been rejected (example). My pinging of @natorion above is probably enough notification of upstream.

ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
Original commit message:

    Quit creating array literal boilerplates from Crankshaft.

    It's such a corner case.

    BUG=

    Review URL: https://codereview.chromium.org/1865013002

    Cr-Commit-Position: refs/heads/master@{nodejs#35346}

Fixes: nodejs#7454
PR-URL: nodejs#7632
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
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.

5 participants