Skip to content

cmd/utils: Add check on hard limit, skip test if below target#15684

Merged
karalabe merged 4 commits into
ethereum:masterfrom
ethersphere:fdlimit_fix
Dec 21, 2017
Merged

cmd/utils: Add check on hard limit, skip test if below target#15684
karalabe merged 4 commits into
ethereum:masterfrom
ethersphere:fdlimit_fix

Conversation

@nolash
Copy link
Copy Markdown
Contributor

@nolash nolash commented Dec 15, 2017

Issue #15683

@karalabe
Copy link
Copy Markdown
Member

This will fail building on Windows since file descriptor limits are handled differently there. I think it's fine to reduce the test target from 4K to 2K. That would solve your issue without requiring fancier changes.

Alternatively you could add a getFdMaxLimit function to the windows, unix and freebsd files to permt retrieving syscall.Rlimit.Max.

@nolash
Copy link
Copy Markdown
Contributor Author

nolash commented Dec 18, 2017

Thanks @karalabe , adjusted.

Comment thread cmd/utils/fdlimit_test.go Outdated
// per this process can be retrieved.
func TestFileDescriptorLimits(t *testing.T) {
target := 4096
var limit syscall.Rlimit
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please avoid using the syscall.Rlimit type here to make the Windows build pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

argh sorry

@karalabe karalabe added this to the 1.8.0 milestone Dec 21, 2017
Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM! Will merge when green. Pushed some minor doc additions.

@karalabe karalabe merged commit 1485281 into ethereum:master Dec 21, 2017
@nonsense nonsense deleted the fdlimit_fix branch August 6, 2018 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants