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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Dec 21, 2017
Merged

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

merged 4 commits into from
Dec 21, 2017

Conversation

nolash
Copy link
Contributor

@nolash nolash commented Dec 15, 2017

Issue #15683

@karalabe
Copy link
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
Contributor Author

nolash commented Dec 18, 2017 •
edited
Loading

Thanks @karalabe , adjusted.

fjl
fjl requested changes Dec 19, 2017
View reviewed changes
cmd/utils/fdlimit_test.go Outdated

// TestFileDescriptorLimits simply tests whether the file descriptor allowance
// per this process can be retrieved.
func TestFileDescriptorLimits(t *testing.T) {
target := 4096
var limit syscall.Rlimit
Copy link
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
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
karalabe
karalabe approved these changes Dec 21, 2017
View reviewed changes
Copy link
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
@5chdn 5chdn mentioned this pull request Jan 30, 2018
Merged
@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
Reviewers

@fjl fjl fjl requested changes

@karalabe karalabe karalabe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nolash @karalabe @fjl