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

lib: fix segfault with --without-intl #21589

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 29, 2018

Node.js segfaults when build with --without-intl due to an oversight
in d13cdd9. This fixes the issue.

This is an alternate to #21587 and was proposed in IRC by @devsnek and also @refack. But @devsnek doesn't have access to GitHub right now, so I'm submitting it to fast-track it and get CI green again.

Speaking of which: 👍 to fast-track.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Node.js segfaults when build with `--without-intl` due to an oversight
in d13cdd9. This fixes the issue.
@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 29, 2018
@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

@nodejs/releasers What is your preference for how to do these kinds of things? Revert the offending commit and then re-land? Or a patch fix like this one is fine? Or it doesn't matter either way?

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

(The revert commit causes a test to fail so it will take some tweaking if we want to go that route. So I'd prefer this PR over a revert at this point...)

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

Resume Build for CI: https://ci.nodejs.org/job/node-test-pull-request/15667/

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

arm-fanned Rebuild: https://ci.nodejs.org/job/node-test-binary-arm/2095/

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

arm-fanned rebuild (https://ci.nodejs.org/job/node-test-binary-arm/2095/) is green.

Just waiting for node-test-commit-linux to finish. Everything is green except fedora-latest-x64 which will hopefully be finished running in another 10 minutes or so....

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

https://ci.nodejs.org/job/node-test-commit-linux/19844/nodes=fedora-latest-x64/console is stuck.

@refack For what it's worth, I often find the console on that shows the querystring.json line in compile for a half hour or more, and then suddenly finishes all at once, like it's buffering the entire console.

Always stops on querystring.json or somewhere near that. Can take up to an hour and twenty minutes and still come back green. Sometimes it only takes 12 minutes though. So...
¯\(ツ)

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

@refack: It's doing it right now...stuck at all.json and will probably stay there for several minutes or longer, then suddenly everything will appear all at once. https://ci.nodejs.org/job/node-test-commit-linux/19847/nodes=fedora-latest-x64/console

@refack
Copy link
Contributor

refack commented Jun 29, 2018

I've seen that... But it was at 37 minutes, and it felt unhealthy...
We should figure that thing out...

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

The one host under Linux that wasn't green was green this time. There is another host that is taking a long time, but that platform was green last time. Given the CI situation and the fast-track approval, I think piecing together a green CI from different places like that is acceptable. I'm going to land this. (Plus, this is easy to revert since it's a one-line change.)

@Trott
Copy link
Member Author

Trott commented Jun 29, 2018

Landed in 1c30343

@Trott Trott closed this Jun 29, 2018
Trott added a commit to Trott/io.js that referenced this pull request Jun 29, 2018
Node.js segfaults when build with `--without-intl` due to an oversight
in d13cdd9. This fixes the issue.

PR-URL: nodejs#21589
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jun 30, 2018
Node.js segfaults when build with `--without-intl` due to an oversight
in d13cdd9. This fixes the issue.

PR-URL: #21589
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
@Trott Trott deleted the intl-fix branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants