Skip to content

Travis has an issue with the C build? #218

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

Closed
matrixise opened this issue Feb 7, 2018 · 9 comments
Closed

Travis has an issue with the C build? #218

matrixise opened this issue Feb 7, 2018 · 9 comments

Comments

@matrixise
Copy link
Member

Hi all,

Today I have worked on this issue https://bugs.python.org/issue1100942 and I was waiting for the build of my PR (python/cpython#5578) and I was really surprised to see 57sec for a test.

Files changed: 
Doc/library/datetime.rst Lib/_strptime.py Lib/datetime.py Lib/test/datetimetester.py Misc/NEWS.d/next/Core and Builtins/2018-02-07-10-55-58.bpo-1100942.md4agI.rst Modules/_datetimemodule.c
Only docs were updated, stopping build process.

I have modified one .c file, three .py files and it has only detected the change in the documentation.

Here is the job on Travis, I am really surprised.
https://travis-ci.org/python/cpython/jobs/338426030

Is there an explanation?

@brettcannon ?

@matrixise
Copy link
Member Author

matrixise commented Feb 7, 2018

Here is my test:
bpo-1100942 is the branch of this PR: python/cpython#5578

#!/usr/bin/env bash

files_changed=$(git diff --name-only bpo-1100942) 

echo $(git diff --name-only bpo-1100942 | grep -vE '(\.rst$)|(^Doc)|(^Misc)')
has_code=$(git diff --name-only bpo-1100942 | grep -qvE '(\.rst$)|(^Doc)|(^Misc)')

if ! $has_code
then
    echo "HasCode: Only docs updated"
else
    echo "HasCode: We can update"
fi

if ! echo $files_changed | grep -qvE '(\.rst$)|(^Doc)|(^Misc)'
then
    echo "FilesChanged: Only docs updated"
else
    echo "FilesChanged: We can update"                                                        
fi

and the output:

Lib/_strptime.py Lib/datetime.py Lib/test/datetimetester.py Modules/_datetimemodule.c
HasCode: We can update
FilesChanged: Only docs updated

When I execute the code of Travis, I have the result, Only docs updated but when I execute the git diff with the grep command, I have the right result.

@matrixise
Copy link
Member Author

@brettcannon I have changed the condition with a flat condition, without any variable and the worker of Travis runs the tests

Here is my PR for review https://github.com/python/cpython/pull/5580/files

my 2cents...

@berkerpeksag
Copy link
Member

python/cpython@b2ec361 (see #14 (comment) for details) might be the culprit. @ammaraskar do you have some time to investigate this?

@ammaraskar
Copy link
Member

Oh this is most likely just a quoting issue around the variable, trying changing the line to:

if ! echo "$files_changed" | grep -qvE '(\.rst$)|(^Doc)|(^Misc)'

instead of

if ! echo $files_changed | grep -qvE '(\.rst$)|(^Doc)|(^Misc)'

@matrixise
Copy link
Member Author

I have created this issue https://bugs.python.org/issue32802 and the associated PR python/cpython#5589 . This PR can be back ported to 3.6 & 3.7

@Mariatta
Copy link
Member

Mariatta commented Feb 8, 2018

Thanks @matrixise! Good catch and thanks for fixing.
I've merged that PR and now waiting for the backports to be finished.

I wonder if other open PRs should be rebased to include the change in python/cpython#5589?

@ammaraskar
Copy link
Member

ammaraskar commented Feb 8, 2018

I wonder if other open PRs should be rebased to include the change in python/cpython#5589?

A rebase won't be required, travis pulls in latest changes and applies the changes to the latest codebase (which should include the new travis.yml). A rebuild should do.

Someone who has administrator access over the python travis should be able to trigger a rebuild. From a quick look it would be every RP after this one: python/cpython#5449. Another view on travis that should help is this: https://travis-ci.org/python/cpython/pull_requests

Of course PRs that change only documentation don't need a rebuild, it should also be easy to see if the full build went through by the time taken.

@brettcannon
Copy link
Member

can this issue be closed?

@berkerpeksag
Copy link
Member

I think we close this now. Thank you everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants