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

test: remove --no-crankshaft #14531

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

The option --no-crankshaft is no longer available as of V8 6.0

This needs to land to get the test suite working for #14004

The option `--no-crankshaft` is no longer available as of V8 6.0
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 28, 2017
@MylesBorins
Copy link
Contributor Author

Would like to see this land in less than 48 hours if there are no objections

@targos targos mentioned this pull request Jul 28, 2017
8 tasks
@Fishrock123
Copy link
Contributor

@MylesBorins this properly works without #14004?

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

@MylesBorins
Copy link
Contributor Author

@Fishrock123 the original change works properly on master afaict. I rebased #14004 and now we have the failures
¯\_(ツ)_/¯

@mscdex
Copy link
Contributor

mscdex commented Jul 28, 2017

@Fishrock123 V8 5.9 (currently in master) already has crankshaft disabled by default.

This PR LGTM.

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jul 28, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems like a sufficiently small change that fast-tracking is 👌 by me.

@Fishrock123
Copy link
Contributor

odd, the CI never updated this even though it completed successfully...

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 28, 2017
The option `--no-crankshaft` is no longer available as of V8 6.0

PR-URL: nodejs#14531
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott
Copy link
Member

Trott commented Jul 28, 2017

Landed in 7128e3c

@Trott Trott closed this Jul 28, 2017
@Trott
Copy link
Member

Trott commented Jul 29, 2017

odd, the CI never updated this even though it completed successfully...

@Fishrock123 CI has been that way for at least two weeks now, perhaps longer. I think @phillipj was pinged about it at some point, but if you want to take a look and see if you can figure out why the bot + CI isn't updating widgets anymore, that would be great.

addaleax pushed a commit that referenced this pull request Jul 29, 2017
The option `--no-crankshaft` is no longer available as of V8 6.0

PR-URL: #14531
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins MylesBorins deleted the no-no-crankshaft branch November 14, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants