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

build: unify setting of ulimit and stack_size #687

Merged
merged 3 commits into from Sep 6, 2017

Conversation

Projects
None yet
2 participants
@myitcv
Contributor

myitcv commented Aug 26, 2017

Waiting for the Go 1.9 branch to land first - DONE in #651

Picking up on @shurcooL's comment in #669 (comment).

We've started randomly seeing std library tests tip over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run.

build: unify setting of ulimit and stack_size
Picking up on @shurcooL's comment in
#669 (comment).

We've started randomly seeing std library tests tip over the V8 stack
size limit, causing text/template TestMaxExecDepth to fail randomly
locally and on CI. This had previously been addressed by @neelance in
1f89545; the --stack_size flag was passed to NodeJS which in turn passed
the value onto V8. But per
nodejs/node#14567 (comment) it
was pointed out that the value of ulimit -s must be >= the value of
--stack_size for the --stack_size to make any sort of sense. Hence this
commit also harmonises the setting of ulimit -s during the CI test run
with the value of --stack_size that is passed to NodeJS (which in turn
passes that value onto V8) when running either gopherjs test or gopherjs
run.
@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 27, 2017

Contributor

@shurcooL @neelance this is ready for review.

Contributor

myitcv commented Aug 27, 2017

@shurcooL @neelance this is ready for review.

@dmitshur

Thanks for factoring out this PR.

I have a question about ulimit -s, and a lot of small grammar suggestions for that comment. See inline comments.

Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated tool.go
Show outdated Hide outdated circle.yml
@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 27, 2017

Contributor

@shurcooL addressed those points. Let me know if there's anything else.

Contributor

myitcv commented Aug 27, 2017

@shurcooL addressed those points. Let me know if there's anything else.

Adjust documentation and code style.
Try to improve readability, remove unhelpful and out of place details
(they can be found via git blame if needed).

@dmitshur dmitshur requested a review from neelance Sep 6, 2017

@dmitshur

Sorry about the delay, and thanks for working on this again. I've applied some minor wording and style adjustments, but overall I think this looks good and as simple as it can be.

I think we should merge it now and look to see if the same test failures happen again. If anything comes up, we can revisit this.

LGTM.

@dmitshur dmitshur merged commit 38c2151 into gopherjs:master Sep 6, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Sep 6, 2017

Contributor

Thanks @shurcooL

Contributor

myitcv commented Sep 6, 2017

Thanks @shurcooL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment