ECL test sometimes fails to create maxima directory

[gh-timokau]

I just got this test failure (and I'm pretty sure I've seen it before):

File "/nix/store/br51f1j23cnxmbf19yn62rn5pp2rmqdh-sage-src-8.5/src/doc/ca/intro/index.rst", line 492, in doc.ca.intro.index
Failed example:
    solve([x + y == 6, x - y == 4], x, y)
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest doc.ca.intro.index[1]>", line 1, in <module>
        solve([x + y == Integer(6), x - y == Integer(4)], x, y)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/symbolic/relation.py", line 1107, in solve
        m = maxima(f)
      File "sage/misc/lazy_import.pyx", line 354, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3682)
        return self.get_object()(*args, **kwds)
      File "sage/misc/lazy_import.pyx", line 189, in sage.misc.lazy_import.LazyImport.get_object (build/cythonized/sage/misc/lazy_import.c:2347)
        return self._get_object()
      File "sage/misc/lazy_import.pyx", line 221, in sage.misc.lazy_import.LazyImport._get_object (build/cythonized/sage/misc/lazy_import.c:2584)
        self._object = getattr(__import__(self._module, {}, {}, [self._name]), self._name)
      File "/nix/store/mfbhcnzdxiyj831pr3hg1wwh556a6lv3-python-2.7.15-env/lib/python2.7/site-packages/sage/interfaces/maxima_lib.py", line 113, in <module>
        ecl_eval("(set-pathnames)")
      File "sage/libs/ecl.pyx", line 1326, in sage.libs.ecl.ecl_eval (build/cythonized/sage/libs/ecl.c:10224)
        cpdef EclObject ecl_eval(str s):
      File "sage/libs/ecl.pyx", line 1341, in sage.libs.ecl.ecl_eval (build/cythonized/sage/libs/ecl.c:10163)
        o=ecl_safe_eval(o)
      File "sage/libs/ecl.pyx", line 350, in sage.libs.ecl.ecl_safe_eval (build/cythonized/sage/libs/ecl.c:5221)
        raise RuntimeError("ECL says: {}".format(
    RuntimeError: ECL says: Could not create directory "/build/sage-home/.sage/maxima"
    C library error: "File exists"
**********************************************************************

This cannot be reliably reproduced. Probably some parallelism issue (tests were run with 4 threads).

[gh-timokau]

It would be nice to give every test process its own DOT_SAGE directory. Would that be possible? I tried doing that, but got confused as to how the doctest processes are actually started and how isolated they are from the "master" process. I tried setting DOT_SAGE in env.py based on DOCTEST_MODE, but that apparently always is False.

[embray]

I have also argued in the past that the tests should use a clean .sage. It's a problem for some other tests as well. I don't know if there's a good reason not to, other than "nobody's done it yet".

[gh-timokau]

The only reason I could think of is that some weird configurations wouldn't be tested. But I would think that is more of a feature: Having doctests depend on the user configuration makes everything much more difficult and relying on users to pick up issues when testing with their configuration doesn't sound like a particularly good practice either.

[embray]

Replying to gh-timokau:

The only reason I could think of is that some weird configurations wouldn't be tested. But I would think that is more of a feature: Having doctests depend on the user configuration makes everything much more difficult and relying on users to pick up issues when testing with their configuration doesn't sound like a particularly good practice either.

Yes, if there are any bugs resulting from user configuration those should be then reported by users and captured as a regression test: Tests should be isolated as possible and reproducible.

[gh-timokau]

Do you happen to know how practical it would be to implement then? As I mentioned earlier, it seems like DOCTEST_MODE is only set to true *after* env.py was evaluated.

[embray]

I think it would be quite practical; just nobody's been sufficiently motivated enough before now.

Probably somewhere in sage.doctest.forker we would just create a tempdir and set sage.env.DOT_SAGE to that tempdir before running any examples in a test file.

[gh-timokau]

But that would set sage.env.DOT_SAGE for all processes right?

[embray]

No, not if it were done after fork.

[embray]

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

[gh-timokau]

Replying to embray:

I think it would be quite practical; just nobody's been sufficiently motivated enough before now.

Probably somewhere in sage.doctest.forker we would just create a tempdir and set sage.env.DOT_SAGE to that tempdir before running any examples in a test file.

I now remembered why that wouldn't work. There are other variables that depend on sage.env.DOT_SAGE which wouldn't be re-evaluated. We'd probably need to either change env.py to use @propertys or spawn completely independent sage instances.

[gh-timokau]

Although that wouldn't even fix this issue since MAXIMA_USERDIR is set in sage-env, not env.py.

[embray]

I still haven't looked at it that deeply but I suspect you're overthinking it. Though the concern about MAXIMA_USERDIR is definitely valid. That's the kind of problem I wanted to address in #22652, but every time it's come up it's been easier to just implement a narrow fix rather than a general solution.

[gh-timokau]

Repeating my comment I accidentally posted in #22652 here:

Replying to embray:

I still haven't looked at it that deeply but I suspect you're overthinking it.

env.py contains these lines:

_add_variable_or_fallback('PYTHON_EGG_CACHE',   opj('$DOT_SAGE', '.python-eggs'))
_add_variable_or_fallback('SAGE_STARTUP_FILE',  opj('$DOT_SAGE', 'init.sage'))

Which is admittedly way less occurrences than I expected. Still, those would need to be re-evaluated or their usage changed.

Though the concern about MAXIMA_USERDIR is definitely valid. That's the kind of problem I wanted to address in #22652,

#22652 would be amazing. I don't see why it would necessary fix these issues though. Of course it would make sense to tackle both issues at the same time.

but every time it's come up it's been easier to just implement a narrow fix rather than a general solution.

Speaking of narrow fixes, for this specific use-case, for this particular issue that should do it (how do I get that nice patch highlighting on trac?):

diff --git a/src/sage/doctest/forker.py b/src/sage/doctest/forker.py
index 02e18e67e7..115f748727 100644
--- a/src/sage/doctest/forker.py
+++ b/src/sage/doctest/forker.py
@@ -668,7 +668,14 @@ class SageDocTestRunner(doctest.DocTestRunner, object):
 
             try:
                 # Don't blink!  This is where the user's code gets run.
+                # print("MAXIMA_USERDIR is: {}".format(os.environ['MAXIMA_USERDIR']))
+                os.environ['MAXIMA_USERDIR'] = "/tmp/maxima-userdir-{}".format(os.getpid())
                 self.compile_and_execute(example, compiler, test.globs)
             except SystemExit:
                 raise
             except BaseException:

I'll probably adopt that for nix. I don't want to fix the same issue on our build server every week. Should I submit that upstream or is that too "hacky"?

[gh-timokau]

I ended up with ​https://github.com/NixOS/nixpkgs/pull/54285.

[jhpalmieri]

Replying to gh-timokau:

(how do I get that nice patch highlighting on trac?)

On the line right after the triple braces {{{, add the line #!diff. (See also ​https://trac.sagemath.org/wiki/WikiProcessors for more options like #!sh, #!html, etc.)

[jdemeyer]

You seem too much focusing on the doctest framework here: this is not a bug in the doctest framework, so it shouldn't be fixed in the doctest framework.

Ideally, this should be reported to ECL upstream.

[gh-timokau]

They seem to assume only one ecl process is running at a time, which may or may not be a bug.

[embray]

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

[embray]

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

[mkoeppe]

I'm getting a similar failure while building doc-html

[mkoeppe]

The error seems to come from the ECL function SYSTEM::ENSURE-DIRECTORIES-EXIST (​https://gitlab.com/embeddable-common-lisp/ecl/-/blob/develop/src/lsp/mislib.lsp#L277), which should ignore EEXIST because of possible races but does not. But this is likely called via some defsystem facility that is probably also not protected against concurrent use.