Opened 5 years ago
Closed 5 years ago
#23979 closed defect (fixed)
Ignore failure in setrlimit on Cygwin
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.1 |
Component: | porting: Cygwin | Keywords: | |
Cc: | jdemeyer | Merged in: | |
Authors: | Erik Bray | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 90507bf (Commits, GitHub, GitLab) | Commit: | 90507bf5507a26be255a4a231f5b608d7d2efa8e |
Dependencies: | Stopgaps: |
Description
Fixes bug introduced by #23748 that makes it impossible to run the tests on Cygwin.
Change History (11)
comment:1 Changed 5 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
Status: | needs_review → needs_info |
---|
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.
comment:4 Changed 5 years ago by
Component: | PLEASE CHANGE → porting: Cygwin |
---|
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.
comment:5 Changed 5 years ago by
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.
comment:6 Changed 5 years ago by
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.
comment:7 Changed 5 years ago by
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.
comment:8 Changed 5 years ago by
Commit: | f050a342b6bf058dee452e2db0851cddb42c265e → 90507bf5507a26be255a4a231f5b608d7d2efa8e |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
90507bf | Only catch exception from setrlimit on Cygwin
|
comment:9 Changed 5 years ago by
Status: | needs_info → needs_review |
---|
comment:10 Changed 5 years ago by
Reviewers: | → Jeroen Demeyer |
---|---|
Status: | needs_review → positive_review |
comment:11 Changed 5 years ago by
Branch: | u/embray/tests/rlimit → 90507bf5507a26be255a4a231f5b608d7d2efa8e |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Do you know why
setrlimit()
does not work? Is it a Python problem or a Cygwin problem?