#9739: Handle duplicate file basenames when testing multiple files in parallel
-------------------------------+--------------------------------------------
Reporter: mpatel | Owner: mvngu
Type: defect | Status: needs_review
Priority: critical | Milestone: sage-4.7.1
Component: doctest | Keywords: doctest scripts
Work_issues: | Upstream: N/A
Reviewer: Robert Bradshaw | Author: Mitesh Patel, John Palmieri
Merged: | Dependencies:
-------------------------------+--------------------------------------------
Comment(by leif):
''Sorry, atm too tired to look at the whole, only a few remarks before I
again forget them...''
Replying to [comment:63 jhpalmieri]:
> Here is a patch based on Mitesh's; the "delta" patch shows the changes;
these changes include your in-line patch.
Nice, thanks. Even flushing the output.
[[BR]]
> This does not change SAGE_TESTDIR to `tempfile.gettempdir()`: we store
timing information in this directory, so it should not be temporary.
Who cares? ;-)
The odd thing with that is that we then again produce potential race
conditions for the timing files. I think we'd have to use locking then
(which can also cause headaches), perhaps on a follow-up.
IMHO these files also shouldn't be "hidden", and could live in or below
`DOT_SAGE` (if we have to use locking anyway). Moreover, they perhaps
shouldn't get lost if one "manually" sets `SAGE_TESTDIR` to e.g. `/tmp`,
which seems reasonable at least as long as we don't automatically use some
presumably fast filesystem for the really temporary files.
[[BR]]
> It might be a good idea to change `TMP` (as used in `sage-ptest`) to a
temporary directory, but I didn't make this change either, just added a
comnent about it.
Ok, see above; at least documented.
[[BR]]
> The changes: [[BR]]
> - the filename mangling uses the full path of the file rather than
`tempfile.mkstemp`; this should be good enough, especially since we work
in a directory with name determined by the pid and the hostname.
This doesn't help when simultaneously testing the same file from one
`sage-ptest` instance (`SAGE_TEST_ITER`, `SAGE_TEST_GLOBAL_ITER`) if I'm
not wrong; we could mangle '''`sage-doctest`'s''' PID into the
''filename'', too, as I suggested above.
[[BR]]
> - there are now messages printed about the doctesting directory, and
then deleting it at the end.
For readability, I'd move the `print` statements (''"Removing the
directory ..."'', which hopefully don't raise exceptions) out of the `try`
block.
Also, `sage-ptest` should know whether tests failed (or doctesting was
interrupted), in which case we don't have to issue a warning since keeping
the failed doctest files is intentional (and the left files should have
been mentioned in previous messages).
So I wouldn't try to remove the directory if any doctests failed, unless
they were [all] interrupted by Ctrl-C (in which case `sage-doctest` should
immediately remove all temporary files belonging to the aborted doctest,
which it currently doesn't).
----
Btw., unrelated to ''this'' ticket: `sage-doctest` shouldn't sleep for 0.1
seconds (and not continually poll the state of the child process) if the
timeout is in seconds anyway; instead, it should use `signal.alarm()` and
`wait()`.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9739#comment:65>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
