Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_regrtest alters the execution environment on Android #76427

Closed
xdegaye mannequin opened this issue Dec 7, 2017 · 8 comments
Closed

test_regrtest alters the execution environment on Android #76427

xdegaye mannequin opened this issue Dec 7, 2017 · 8 comments
Labels
3.7 tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@xdegaye
Copy link
Mannequin

xdegaye mannequin commented Dec 7, 2017

BPO 32246
Nosy @vstinner, @xdegaye
Superseder
  • bpo-32252: test_regrtest leaves a test_python_* directory in TEMPDIR
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-06-24.13:22:09.409>
    created_at = <Date 2017-12-07.16:01:42.258>
    labels = ['3.7', 'type-bug', 'tests']
    title = 'test_regrtest alters the execution environment on Android'
    updated_at = <Date 2019-06-24.13:22:09.408>
    user = 'https://github.com/xdegaye'

    bugs.python.org fields:

    activity = <Date 2019-06-24.13:22:09.408>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-06-24.13:22:09.409>
    closer = 'vstinner'
    components = ['Tests']
    creation = <Date 2017-12-07.16:01:42.258>
    creator = 'xdegaye'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32246
    keywords = []
    message_count = 8.0
    messages = ['307812', '307813', '307828', '307829', '307838', '307843', '308030', '346397']
    nosy_count = 2.0
    nosy_names = ['vstinner', 'xdegaye']
    pr_nums = []
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '32252'
    type = 'behavior'
    url = 'https://bugs.python.org/issue32246'
    versions = ['Python 3.7']

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    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 :-)
    1. 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 bpo-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.

    @xdegaye xdegaye mannequin added 3.7 tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Dec 7, 2017
    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 7, 2017

    The command 'adb logcat' reports a crash ("Fatal signal 11 (SIGSEGV)") during the execution of the command. This sounds familiar :-)

    See issue bpo-32138 and bpo-26934.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 7, 2017

    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?

    @vstinner
    Copy link
    Member

    vstinner commented Dec 7, 2017

    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 python/cpython#76319: Raising SIGSEGV on Android may not cause a crash.
        return unittest.skipIf(is_android,
                               'raising SIGSEGV on Android is unreliable')(test)

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 8, 2017

    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 bpo-32252 for that distinct problem.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 8, 2017

    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.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Dec 11, 2017

    To debug and reproduce the problem on Android, one must checkout from the bpo-30386 branch c0ca089.

    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 c0ca089... 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 bpo-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"

    @vstinner
    Copy link
    Member

    vstinner commented Jun 24, 2019

    This issue should be fixed by bpo-32252:

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

    bpo-32252: Fix faulthandler_suppress_crash_report() (bpo-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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant