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

5.0.0 breaking changes: --append behavior #880

Closed
gpotter2 opened this issue Dec 15, 2019 · 13 comments
Closed

5.0.0 breaking changes: --append behavior #880

gpotter2 opened this issue Dec 15, 2019 · 13 comments
Labels
Milestone

Comments

@gpotter2
Copy link

@gpotter2 gpotter2 commented Dec 15, 2019

Describe the bug

> Some breaking changes (?) should be documented in
https://coverage.readthedocs.io/en/coverage-5.0/whatsnew5x.html

I'm assuming the --append option is now enabled by default, thus breaking codecov combine. (That's my guess, see below to make up your mind).

Edited: the inconsistency with 4.5.4 seem to be a bug fix.

I haven't seen this documented anywhere. Our test suite broke this morning, and after some troubleshooting It appears that's related to the 5.0.0 update of coverage.py.

We use coverage.py as part of our automated unit testing, here's our basic workflow:

  • Tests are started using tox
  • We run several unit tests using coverage run -a -m [...]
  • coverage combine
  • We call codecov which starts off by calling coverage xml

To Reproduce

  1. What version of Python are you using?
gpotter@vmg:~/github/scapy$ python --version
Python 3.7.4
  1. What version of coverage.py are you using? The output of coverage debug sys is helpful.
For coverage 4.5.4
gpotter@vmg:~/github/scapy$ coverage debug sys
-- sys -------------------------------------------------------
              version: 4.5.4
             coverage: /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages/coverage/__init__.py
          cover_paths: /netdisk/home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages/coverage
          pylib_paths: /netdisk/home/gpotter/.pyenv/versions/3.7.4/lib/python3.7
               tracer: CTracer
 plugins.file_tracers: -none-
  plugins.configurers: -none-
         config_files: .coveragerc
         configs_read: .coveragerc
            data_path: /netdisk/home/gpotter/github/scapy/.coverage
               python: 3.7.4 (default, Sep 11 2019, 21:46:22) [GCC 7.4.0]
             platform: Linux-5.0.0-1025-azure-x86_64-with-debian-buster-sid
       implementation: CPython
           executable: /home/gpotter/.pyenv/versions/3.7.4/bin/python3
                  cwd: /netdisk/home/gpotter/github/scapy
                 path:
                       /home/gpotter/.pyenv/versions/3.7.4/lib/python37.zip
                       /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7
                       /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/lib-dynload
                       /home/gpotter/.local/lib/python3.7/site-packages
                       /home/gpotter/.local/lib/python3.7/site-packages/mcstatus-2.2.1-py3.7.egg
                       /home/gpotter/.local/lib/python3.7/site-packages/dnspython3-1.15.0-py3.7.egg
                       /home/gpotter/.local/lib/python3.7/site-packages/Click-7.0-py3.7.egg
                       /home/gpotter/.local/lib/python3.7/site-packages/six-1.12.0-py3.7.egg
                       /home/gpotter/.local/lib/python3.7/site-packages/dnspython-1.15.0-py3.7.egg
                       /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages
          environment: PYENV_DIR = /home/gpotter/github/scapy
                       PYENV_HOOK_PATH = /home/gpotter/.pyenv/pyenv.d:/usr/local/etc/pyenv.d:/etc/pyenv.d:/usr/lib/pyenv/hooks:/home/gpotter/.pyenv/plugins/pyenv-virtualenv/etc/pyenv.d:/home/gpotter/.pyenv/plugins/pyenv-which-ext/etc/pyenv.d
                       PYENV_ROOT = /home/gpotter/.pyenv
                       PYENV_SHELL = bash
                       PYENV_VERSION = 3.7.4
                       PYENV_VIRTUALENV_INIT = 1
         command_line: /home/gpotter/.pyenv/versions/3.7.4/bin/coverage debug sys
         source_match: -none-
    source_pkgs_match: -none-
        include_match: -none-
           omit_match: /netdisk/home/gpotter/github/scapy/scapy/tools/UTscapy.py
                       /netdisk/home/gpotter/github/scapy/test/*
                       /netdisk/home/gpotter/github/scapy/scapy/modules/six.py
                       /netdisk/home/gpotter/github/scapy/scapy/modules/winpcapy.py
                       /netdisk/home/gpotter/github/scapy/scapy/modules/ethertypes.py
                       /netdisk/home/gpotter/github/scapy/.tox/*
                       /private/*
          cover_match: /netdisk/home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages/coverage
          pylib_match: /netdisk/home/gpotter/.pyenv/versions/3.7.4/lib/python3.7
For coverage 5.0.0
gpotter@vmg:~/github/scapy$ coverage debug sys
-- sys -------------------------------------------------------
                   version: 5.0
                  coverage: /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages/coverage/__init__.py
                    tracer: -none-
                   CTracer: available
      plugins.file_tracers: -none-
       plugins.configurers: -none-
 plugins.context_switchers: -none-
         configs_attempted: .coveragerc
              configs_read: .coveragerc
               config_file: .coveragerc
           config_contents: '[run]\nconcurrency = multiprocessing\nomit =\n    # Scapy specific paths\n    scapy/tools/UTscapy.py\n    test/*\n    # Scapy external modules\n    scapy/modules/six.py\n    scapy/modules/winpcapy.py\n    scapy/modules/ethertypes.py\n    # .tox specific path\n    .tox/*\n    # OS specific paths\n    /private/*\n'
                 data_file: -none-
                    python: 3.7.4 (default, Sep 11 2019, 21:46:22) [GCC 7.4.0]
                  platform: Linux-5.0.0-1025-azure-x86_64-with-debian-buster-sid
            implementation: CPython
                executable: /home/gpotter/.pyenv/versions/3.7.4/bin/python3
              def_encoding: utf-8
               fs_encoding: utf-8
                       pid: 91527
                       cwd: /netdisk/home/gpotter/github/scapy
                      path: /netdisk/home/gpotter/.pyenv/versions/3.7.4/bin
                            /home/gpotter/.pyenv/versions/3.7.4/lib/python37.zip
                            /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7
                            /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/lib-dynload
                            /home/gpotter/.local/lib/python3.7/site-packages
                            /home/gpotter/.local/lib/python3.7/site-packages/mcstatus-2.2.1-py3.7.egg
                            /home/gpotter/.local/lib/python3.7/site-packages/dnspython3-1.15.0-py3.7.egg
                            /home/gpotter/.local/lib/python3.7/site-packages/Click-7.0-py3.7.egg
                            /home/gpotter/.local/lib/python3.7/site-packages/six-1.12.0-py3.7.egg
                            /home/gpotter/.local/lib/python3.7/site-packages/dnspython-1.15.0-py3.7.egg
                            /home/gpotter/.pyenv/versions/3.7.4/lib/python3.7/site-packages
               environment: PYENV_DIR = /home/gpotter/github/scapy
                            PYENV_HOOK_PATH = /home/gpotter/.pyenv/pyenv.d:/usr/local/etc/pyenv.d:/etc/pyenv.d:/usr/lib/pyenv/hooks:/home/gpotter/.pyenv/plugins/pyenv-virtualenv/etc/pyenv.d:/home/gpotter/.pyenv/plugins/pyenv-which-ext/etc/pyenv.d
                            PYENV_ROOT = /home/gpotter/.pyenv
                            PYENV_SHELL = bash
                            PYENV_VERSION = 3.7.4
                            PYENV_VIRTUALENV_INIT = 1
              command_line: /home/gpotter/.pyenv/versions/3.7.4/bin/coverage debug sys
           sqlite3_version: 2.6.0
    sqlite3_sqlite_version: 3.22.0
  1. What versions of what packages do you have installed? The output of pip freeze is helpful.
  2. What code are you running? (Most likely not useful,) this commit
  3. What commands did you run?
coverage --version
# those commands are a demo: we just start scapy and stop it. You can most likely use anything
echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
ls -alF | grep ".coverage"
coverage combine
echo $?

Expected behavior

Here's what happens when running the code displayed above:

  • On 5.0.0:
gpotter@vmg:~/github/scapy$ coverage --version
Coverage.py, version 5.0 with C extension
Full documentation is at https://coverage.readthedocs.io
gpotter@vmg:~/github/scapy$ echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
gpotter@vmg:~/github/scapy$ echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
gpotter@vmg:~/github/scapy$ ll | grep ".coverage"
-rw-r--r--  1 gpotter gpotter 81920 Dec 15 13:29 .coverage
-rw-rw-r--  1 gpotter gpotter   299 Dec  6 22:41 .coveragerc
gpotter@vmg:~/github/scapy$ coverage combine
No data to combine
gpotter@vmg:~/github/scapy$ echo $?
1
  • On 4.5.4:
gpotter@vmg:~/github/scapy$ coverage --version
Coverage.py, version 4.5.4 with C extension
Documentation at https://coverage.readthedocs.io
gpotter@vmg:~/github/scapy$ echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
gpotter@vmg:~/github/scapy$ echo "exit()" | coverage run -a -m scapy > /dev/null 2>&1
gpotter@vmg:~/github/scapy$ ll | grep ".coverage"
-rw-rw-r--  1 gpotter gpotter 82949 Dec 15 13:30 .coverage.vmg.90662.854455
-rw-rw-r--  1 gpotter gpotter 82949 Dec 15 13:30 .coverage.vmg.90762.195813
-rw-rw-r--  1 gpotter gpotter   299 Dec  6 22:41 .coveragerc
gpotter@vmg:~/github/scapy$ coverage combine
gpotter@vmg:~/github/scapy$ echo $?
0
Our `.coveragerc` file
[run]
concurrency = multiprocessing
omit =
    # Scapy specific paths
    scapy/tools/UTscapy.py
    test/*
    # Scapy external modules
    scapy/modules/six.py
    scapy/libs/winpcapy.py
    scapy/libs/ethertypes.py
    # .tox specific path
    .tox/*
    # OS specific paths
    /private/*

As shown, the return code is 1 on 5.0.0, and 0 on 4.5.4. This explains why our tests suddently failed starting today.

Should this be documented ? Is this behavior expected ?

Thanks for your time

@gpotter2 gpotter2 added the bug label Dec 15, 2019
gpotter2 added a commit to gpotter2/scapy that referenced this issue Dec 15, 2019
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 15, 2019

The problem isn't that --append is the default. --multiprocessing=concurrency forces parallel mode for subprocesses. In coverage 4.x, it also forced it for the main process, so the data file would have unique names, as you are seeing for your main process. In coverage 5.0, the main data file no longer gets the unique name.

Can you try adding parallel = True to your .coveragerc to see if it fully fixes the problem?

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 15, 2019

Oh, actually the main process gets a parallelized data file unless append mode is in effect, in which case it doesn't.

gpotter2 added a commit to gpotter2/scapy that referenced this issue Dec 15, 2019
@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 15, 2019

Thanks for the note. With the commit above, I reversed the previous change to use parallel=True.
I now get

Can't append to data files in parallel mode.

When running the coverage -a -m [...] calls (see the PR and the failing tests)

@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 15, 2019

I realize the current behavior actually makes sense. All data is appended with -a (I forgot that meant --append...), how did I miss that.

The previous behavior was the buggy one: despite the -a, it required a combine. It is now fixed..

Therefore it's all good! I'll stick to the "allow failure" trick to remain backward compatible and call it a day

Thanks for the help

gpotter2 added a commit to gpotter2/scapy that referenced this issue Dec 15, 2019
@gpotter2 gpotter2 changed the title 5.0.0 breaking changes: --append enabled by default? 5.0.0 breaking changes: --append behavior Dec 15, 2019
@gpotter2 gpotter2 closed this Dec 15, 2019
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 15, 2019

I'm not sure this should be closed yet. Are you using multiprocessing in your code? What error are you having to ignore in the combine step?

@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 15, 2019

  • yes we use multiprocessing, which is why it was in .coveragerc
  • we get No data to combine in 5.0.0.

In 5.0.0, the -a parameter apparently works: at the end of the tests, there only is a single .coverage file. In 4.5.4 however, we get the multiple .coverage.xxx.xxxx files even with the -a flag, which is why we used coverage combine.

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 15, 2019

If combine is reporting "No data to combine" then is it doing anything? Are you sure this is working?

@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 15, 2019

About coverage combine:

  • It's not working on 5.0.0 because files were correctly appended (-a worked) during codecov run
  • it was working in 4.5.4 because files were not appended despite the -a

So by fixing this issue, 5.0 broke our tests. It's on us then

@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 15, 2019

I really appreciate your willingness to own this yourself, but I'm still trying to understand what your solution was. If you have to ignore an error status from "coverage combine", then it seems like combine isn't doing anything.

@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 16, 2019

Sorry for being unclear, to summarize:

  • the append option is broken in 4.5.4 when using multiprocessing concurrency (at least). Files are not appended, and require a manual call to coverage combine. We used to do that.
  • in 5.0.0, files are now properly appended even with multiprocessing concurrency. Therefore, calling the combine would return "no data to combine".

Because we want to remain compatible with 4.5.4 (installed by default on Travis), we'll keep the failing call but ignore it.

I don't know enough of coverage.py to help you further :/ see the first post if you want some code to reproduce it

nedbat added a commit that referenced this issue Dec 17, 2019
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 17, 2019

@gpotter2 I hope this doesn't cause more turmoil for you, but I decided that 5.0 was broken in this regard. "coverage run --concurrency=multiprocessing" should always produce all data files with a parallel-mode suffix. In 5.0, "--append" broke that. I've fixed in it 842f585.

@nedbat nedbat added this to the 5.0.1 milestone Dec 17, 2019
@gpotter2

This comment has been minimized.

Copy link
Author

@gpotter2 gpotter2 commented Dec 17, 2019

Thanks a lot, if it makes things consistent it's all good for us !

I also just understood why it totally made sense to have append mode disabled when using multiprocessing concurrency 😄 it could lead to bad racing issues..

gpotter2 added a commit to gpotter2/scapy that referenced this issue Dec 17, 2019
@nedbat nedbat added the fixed label Dec 22, 2019
@nedbat

This comment has been minimized.

Copy link
Owner

@nedbat nedbat commented Dec 22, 2019

This is now available in coverage==5.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.