classification
Title: test_regrtest alters the execution environment on Android
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.7
process
Status: closed Resolution: duplicate
Dependencies: Superseder: test_regrtest leaves a test_python_* directory in TEMPDIR
View: 32252
Assigned To: Nosy List: vstinner, xdegaye
Priority: normal Keywords:

Created on 2017-12-07 16:01 by xdegaye, last changed 2019-06-24 13:22 by vstinner. This issue is now closed.

Messages (8)
msg307812 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-07 16:01
Sorry, this is a bit involved :-(
BTW all the tests except this one pass (ctypes is disabled on x86_64 and arm64) on Android API 24 for x86, x86_64, armv7 and arm64 :-)

Description:
-----------
There are two different cases:
1) When buildbottest is tweaked to run only test_regrtest and is run remotely from the build system, the error occurs systematically and is:

      Warning -- files was modified by test_regrtest
        Before: []
        After:  ['test_python_2535/']

Here is the command that is run remotely (through adb), the run_tests.py file is Tools/scripts/run_tests.py from the src tree that has been pushed to the emulator by buildbottest:

python /data/local/tmp/python/bin/run_tests.py -j 1 -u all -W --slowest --fail-env-changed --timeout=900 -j2 test_regrtest

The command also fails when replacing '-j2' with '-j1'.

In that case:
* There is no 'test_support_*' directory left over in the TEMP directory, the directory is clean.
* The command 'adb logcat' reports a crash ("Fatal signal 11 (SIGSEGV)") during the execution of the command. This sounds familiar :-)


2) When this same command is run on the emulator (i.e. the user is remotely logged in with the command 'adb shell'), the environment is never altered (never == about 20 attempts to raise the problem).

In that case:
* A 'test_support_*' directory is left in the TEMP directory and it is empty.

The fact that the TEMP directory is not clean already happens when running buildbottest natively with the standard Python Makefile (no one has noticed it yet I guess) and the directory contains a core file. This is another bug, related to this one and it provided a much welcome hint to use 'adb logcat' and for a work-around :-)
Maybe this last bug is related to issue 26295 ?

Work-around
-----------
* The problem does not happen when skipping ArgsTestCase.test_crashed for Android in the same manner that tests that raise SIGSEGV are skipped in test_faulthandler. And there is no 'test_support_*' directory left over in the TEMP directory.
msg307813 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-07 16:07
> The command 'adb logcat' reports a crash ("Fatal signal 11 (SIGSEGV)") during the execution of the command. This sounds familiar :-)

See issue #32138 and #26934.
msg307828 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-12-07 23:12
> The command 'adb logcat' reports a crash ("Fatal signal 11 (SIGSEGV)") during the execution of the command. This sounds familiar :-)

test_crashed() does crash on SIGGEV on purpose. It tests how regrtest behaves when a worker does crash. The exacty signal doesn't matter.

This function uses faulthandler._sigsegv() which is supposed to "suppress crash report": see faulthandler_suppress_crash_report() in Modules/faulthandler.c, but also SuppressCrashReport in test/support/__init__.py.

Maybe we need to disable the Android crash reporter as well?
msg307829 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-12-07 23:20
> The fact that the TEMP directory is not clean already happens when running buildbottest natively with the standard Python Makefile (no one has noticed it yet I guess) and the directory contains a core file.

The root issue is likely the core file. I got a similar issue on FreeBSD when an unit test crashed on purpose but forgot to suppress crash report.

You should either find a way to disable the creation of core file when using faulthandler_suppress_crash_report() and/or SuppressCrashReport, or skip the test on Android.

I'm fine with skipped the test on Android, since the test currently uses faulthandler._sigsegv() which is already skipped on Android in test_faulthandler:

def skip_segfault_on_android(test):
    # Issue #32138: Raising SIGSEGV on Android may not cause a crash.
    return unittest.skipIf(is_android,
                           'raising SIGSEGV on Android is unreliable')(test)
msg307838 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-08 08:53
> The fact that the TEMP directory is not clean already happens when running buildbottest natively with the standard Python Makefile (no one has noticed it yet I guess) and the directory contains a core file.

I should have written "happens on linux" instead of "happens when running buildbottest natively with the standard Python Makefile". I have now created issue 32252 for that distinct problem.
msg307843 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-08 09:42
I agree that test_crashed() should be skipped on Android anyway and I will create a PR to skip the test.

If I understand correctly, there may be another nasty bug that is revealed when test_regrstest fails with the environment altered:

    How is it possible that the new file that alters the environment be a test_python_* directory (see my initial post) when all the tests are run inside a TEMPDIR/test_python_* directory. The 'files' environment is checked within a TEMPDIR/test_python_* and not within TEMPDIR, no ?

I found a new point that may help understand this problem:

* On Android TEMPDIR is built from tempfile.gettempdir() that uses the TMPDIR environment variable which is set by Android to /data/local/tmp. The tests are run in /data/local/tmp/python [1], this is also the value of $SYS_EXEC_PREFIX and thus where are installed Python machine-specific binaries.

* When TMPDIR is set to /data/local/tmp/python/tmp, which makes more sense here, test_regrstest is ok and does not change the environment.

[1] This is the only location where a shell user may have both write and exec permissions, the Android applications have those permissions here too and in their own dedicated locations on /data/data.
msg308030 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-12-11 11:14
To debug and reproduce the problem on Android, one must checkout from the bpo-30386 branch https://github.com/python/cpython/pull/1629/commits/c0ca089220cd39851d7625b55510be63b340e188.

faulthandler._sigsegv() does crash the crasher in test_crashed, so test_crashed must not be disabled.

> How is it possible that the new file that alters the environment be a test_python_* directory (see my initial post) when all the tests are run inside a TEMPDIR/test_python_* directory.

Both test_crashed tests are run in nested tests directories (i.e. in $TEMPDIR/test_python_*/test_python_*) on Android. As a consequence the test_python_* directory that is not removed by the crasher because it crashed, is reported by test_crashed as altering the environment. This also explains the other points raised previously.

A remote interactive shell is run by the 'adb shell' command and the TMPDIR environment variable is set in that shell. A command may be run remotely with the command 'adb shell /path/to/command' and the TMPDIR variable is *not set* (but most if not all the other shell environment variables are set) and since on Android /tmp does not exist, then tempfile.gettempdir() uses the current directory. This explains why the test directories are doubly nested.

On the commit that is the child of commit c0ca089220cd39... which we are using here, TMPDIR has been made to be set in the script that runs Tools/script/run_tests.py and so it does fix inadvertently the issue for the reason explained in the previous paragraph.

This bug can only exist on platforms where tempfile.gettempdir() falls back to using the current directory because '/tmp', '/var/tmp' and '/usr/tmp' do not exist and where none of the 'TMPDIR', 'TEMP', 'TMP' variables is set. Therefore we can either close this issue as not a bug or use the following patch that fixes the problem, it is similar to the fix made in issue 15300:

diff --git a/Lib/test/test_regrtest.py b/Lib/test/test_regrtest.py
index 8364767b3a..3a213e12c4 100644
--- a/Lib/test/test_regrtest.py
+++ b/Lib/test/test_regrtest.py
@@ -468,10 +468,13 @@ class BaseTestCase(unittest.TestCase):
             input = ''
         if 'stderr' not in kw:
             kw['stderr'] = subprocess.PIPE
+        savedcwd = (support.SAVEDCWD if
+                    any(x.startswith('-j') for x in args) else None)
         proc = subprocess.run(args,
                               universal_newlines=True,
                               input=input,
                               stdout=subprocess.PIPE,
+                              cwd=savedcwd,
                               **kw)
         if proc.returncode != exitcode:
             msg = ("Command %s failed with exit code %s\n"
msg346397 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-24 13:22
This issue should be fixed by bpo-32252:

commit 48d4dd974f0c8d47c54990eedd322b96b19c60ec
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Mon Dec 11 13:57:12 2017 +0100

    bpo-32252: Fix faulthandler_suppress_crash_report() (#4794)
    
    Fix faulthandler_suppress_crash_report() used to prevent core dump files
    when testing crashes. getrlimit() returns zero on success.


If it's not the case. Please open the issue.
History
Date User Action Args
2019-06-24 13:22:09vstinnersetstatus: open -> closed
superseder: test_regrtest leaves a test_python_* directory in TEMPDIR
messages: + msg346397

resolution: duplicate
stage: needs patch -> resolved
2017-12-11 11:14:31xdegayesetmessages: + msg308030
2017-12-09 15:42:06xdegayelinkissue26865 dependencies
2017-12-08 09:42:16xdegayesetmessages: + msg307843
2017-12-08 08:53:56xdegayesetmessages: + msg307838
2017-12-07 23:20:32vstinnersetmessages: + msg307829
2017-12-07 23:12:45vstinnersetmessages: + msg307828
2017-12-07 16:07:23xdegayesetmessages: + msg307813
2017-12-07 16:01:42xdegayecreate