Do not attempt to write out a depfile on failure

[eli-schwartz]

This would be pretty useless as it cannot be used -- the output file does not exist either. But as it happens, on error, the output file is reset to None, so instead we triggered a python traceback while trying to write a depfile for os.path.relpath(None, cwd) that was written to None+'.dep'

Discovered when trying to help debug pandas-dev/pandas#49115 (comment)

[scoder]

Seems worth adding a simple test for this where we try to compile an invalid module and check for the depfile afterwards.

[eli-schwartz]

Wouldn't the test pass before this change anyway? The depfile can't be created, this PR just avoids an internal error caused by trying.

[matusvalo]

@eli-schwartz we can do a test that checks whether compilation fails but cython output does not contain Traceback string in stderr (hence it does not crash). I created following file: tests/build/depfile_compilation_failed.srctree:

"""
PYTHON -c 'import os; os.makedirs("builddir/pkg")'
PYTHON check.py
"""

######## check.py ########

import os.path
import subprocess

r = subprocess.run(["CYTHON", "-M", "pkg/test.pyx", "-l", "-o", "builddir/pkg/test.c"], capture_output=True)
assert b'Traceback' not in r.stderr
assert not os.path.exists(os.path.join("builddir", "pkg", "test.c.dep"))
assert not os.path.exists(os.path.join("builddir", "pkg", "test.c"))

######## pkg/__init__.py ########


######## pkg/test.pyx ########

TEST = "pkg.test"

cccccccdef object get_str():
   return TEST

Following test fails on the master and pass on the PR branch.

[eli-schwartz]

We could, yes, but is it standard practice to create unittests to check whether a traceback occurs?

Because that was my whole question -- not how to test it, but whether there is something worth testing. (Especially considering that as a test suite grows larger, its runtime gets longer, and for a project where the tests involve forking the cost of new tests are disproportionate.)

If that's something the cython project wants, I'm happy to oblige, but I don't want to assume (especially since in my own projects I'd prefer to avoid such tests).

[0dminnimda]

We can not fork but cythonize in the test code, for example Cython/Build/Tests/TestCyCache.py
Regarding sensibility of such tests .. IMHO, if it's the expected behavior and if it's changed something can break than it's better be tested, regardless of the ways how to do it and the fact that it'll take time to test.
Tests exists so that if something breaks them we'll know about it immediately and not when user suddenly cannot do something
So I'd say it's a no factor

[eli-schwartz]

IMHO, if it's the expected behavior and if it's changed something can break than it's better be tested, regardless of the ways how to do it and the fact that it'll take time to test.
Tests exists so that if something breaks them we'll know about it immediately and not when user suddenly cannot do something

Exactly. :)

And this isn't a change in expected behavior, and the user doesn't stop being able to do something... it's just testing whether something that fails, does so with a proper error message or else with a confusing internal error in the teardown handling code.

There's no detectable difference between the two other than scanning stderr (the failing return code is the same, the absence of output files is the same) which is why the suggested test case has to hardcode the word "Traceback" in it.

[0dminnimda]

whether something that fails, does so with a proper error message or else with a confusing internal error in the teardown handling code.

Ok, I see now
Although I would find ensuring nice user experience important as well

[scoder]

Thanks