Opened 4 years ago

Closed 4 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:

Status badges

Description

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

Change History (11)

comment:1 Changed 4 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by jdemeyer

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

comment:3 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to 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 4 years ago by embray

  • Component changed from PLEASE CHANGE to 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 4 years ago by embray

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 4 years ago by jdemeyer

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 4 years ago by embray

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 4 years ago by git

  • Commit changed from f050a342b6bf058dee452e2db0851cddb42c265e to 90507bf5507a26be255a4a231f5b608d7d2efa8e

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

​90507bfOnly catch exception from setrlimit on Cygwin

comment:9 Changed 4 years ago by embray

  • Status changed from needs_info to needs_review

comment:10 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:11 Changed 4 years ago by vbraun

  • Branch changed from u/embray/tests/rlimit to 90507bf5507a26be255a4a231f5b608d7d2efa8e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.