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

Ignore failure in setrlimit on Cygwin #23979

Closed
embray opened this issue Oct 6, 2017 · 10 comments
Closed

Ignore failure in setrlimit on Cygwin #23979

embray opened this issue Oct 6, 2017 · 10 comments

Comments

@embray
Copy link
Contributor

embray commented Oct 6, 2017

Fixes bug introduced by #23748 that makes it impossible to run the tests on Cygwin.

CC: @jdemeyer

Component: porting: Cygwin

Author: Erik Bray

Branch/Commit: 90507bf

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23979

@embray embray added this to the sage-8.1 milestone Oct 6, 2017
@jdemeyer
Copy link

jdemeyer commented Oct 6, 2017

comment:2

Do you know why setrlimit() does not work? Is it a Python problem or a Cygwin problem?

@jdemeyer
Copy link

jdemeyer commented Oct 6, 2017

comment:3

I'd like to understand why it doesn't work and check that condition instead of blindly ignoring exceptions. Or, if that doesn't work, only ignore the exception on Cygwin.

@embray
Copy link
Contributor Author

embray commented Oct 9, 2017

comment:4

setrlimit() works for some limits on Cygwin but not all. In particular RLIMIT_AS does not work. But the only effect is to return -1 and set EINVAL, so there's no particular way to distinguish this from other failures that can occur setting this limit (in fact the error message that Python gives in this case is misleading). I suspect this won't be 100% portable in all other cases either; tbh I don't know what motivated this change and the 3300MB setting is pretty arbitrary anyways, so I think it's fine to ignore it if it fails.

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:5

I could raise an exception if not on Cygwin. It just seems to me though like something that should fail quietly, or at most with a warning. It shouldn't just crash the test runner if setrlimit() fails for some reason.

@jdemeyer
Copy link

comment:6

This is only for the doctester, not for running Sage itself. When testing, it is fair to be more strict. You typically want to know when something goes wrong, even if that thing is rather innocent.

So yes, I would be happier to ignore the exception only on Cygwin.

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:7

Right but what I'm saying is the behavior of setting the memory limit to 3300 MB is pretty arbitrary and not something most people running the doctester is even going to be aware is happening, so if for some reason it fails it's not very nice if the doctester just crashes.

Another thing that's unfortunate is that there's no way to distinguish a ValueError when passing an unsupported limit, versus a ValueError when the limits themselves are invalid for some reason. It's not the best interface.

Well, we can try squelching it just in Cygwin for now. I'll be curious to see if this causes problems on any other platforms. Perhaps it won't.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

Changed commit from f050a34 to 90507bf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

90507bfOnly catch exception from setrlimit on Cygwin

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Oct 16, 2017

Changed branch from u/embray/tests/rlimit to 90507bf

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

No branches or pull requests

3 participants