Travis: failed build reported as successful

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Travis: failed build reported as successful

Mojca Miklavec-2
Dear Zero King,

Why is this build reported as successful?

https://travis-ci.org/macports/macports-ports/jobs/248896726

Also: can it be that we're experiencing troubles due to an unusual
$HOME variable?

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
On Fri, Jun 30, 2017 at 10:51:54PM +0200, Mojca Miklavec wrote:
>Dear Zero King,
>
>Why is this build reported as successful?
>
>https://travis-ci.org/macports/macports-ports/jobs/248896726

1. Lint error was ignored. I didn't fix it because "file delete -force"
errors aren't fixed.

2. `portindex` failed to parse the Portfile but didn't return an error.
How can I throw an error when `portindex` failed or printed "Failed to
parse file" while keeping the output in Travis logs (in Bash)?

>Failed to parse file lang/llvm-4.0/Portfile: wrong # args: should be "sha256 action ?file?"


>Also: can it be that we're experiencing troubles due to an unusual
>$HOME variable?

Does Travis environment have an unusual $HOME?

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Mojca Miklavec-2
On 1 July 2017 at 04:33, Zero King wrote:

> On Fri, Jun 30, 2017 at 10:51:54PM +0200, Mojca Miklavec wrote:
>>
>> Dear Zero King,
>>
>> Why is this build reported as successful?
>>
>> https://travis-ci.org/macports/macports-ports/jobs/248896726
>
> 1. Lint error was ignored. I didn't fix it because "file delete -force"
> errors aren't fixed.

OK.

> 2. `portindex` failed to parse the Portfile but didn't return an error.
> How can I throw an error when `portindex` failed or printed "Failed to
> parse file" while keeping the output in Travis logs (in Bash)?

I tried to figure out where you call (anything that calls) portindex,
but no luck :).

If any command that you ran failed, your program should exit with
non-zero status at the end, but I guess you already know that.

>> Failed to parse file lang/llvm-4.0/Portfile: wrong # args: should be
>> "sha256 action ?file?"
>
>> Also: can it be that we're experiencing troubles due to an unusual
>> $HOME variable?
>
> Does Travis environment have an unusual $HOME?

Reading those logs I had the impression that
/opt/local/var/macports/home might be used, but I'm not sure.

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
On Sat, Jul 01, 2017 at 07:51:12AM +0200, Mojca Miklavec wrote:
>I tried to figure out where you call (anything that calls) portindex,
>but no luck :).

https://github.com/macports/macports-ports/blob/master/_ci/bootstrap.sh#L18

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Mojca Miklavec-2
On 1 July 2017 at 09:04, Zero King <[hidden email]> wrote:
> On Sat, Jul 01, 2017 at 07:51:12AM +0200, Mojca Miklavec wrote:
>>
>> I tried to figure out where you call (anything that calls) portindex,
>> but no luck :).
>
>
> https://github.com/macports/macports-ports/blob/master/_ci/bootstrap.sh#L18

I'm sorry, I was looking at runner forever ...

I would say that
    portindex || exit 1
should work.

When individual steps could fail and when I wanted to proceed
executing the script to the end, I usually wrote a function that
checked for failure status and only reported / generated a failure at
the end. But I guess you can safely exit if portindex fails
(potentially add some explicit sentence saying so if it's not clear
from the error message).

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Mojca Miklavec-2
On 1 July 2017 at 09:21, Mojca Miklavec wrote:

> On 1 July 2017 at 09:04, Zero King wrote:
>> On Sat, Jul 01, 2017 at 07:51:12AM +0200, Mojca Miklavec wrote:
>>>
>>> I tried to figure out where you call (anything that calls) portindex,
>>> but no luck :).
>>
>> https://github.com/macports/macports-ports/blob/master/_ci/bootstrap.sh#L18
>
> I would say that
>     portindex || exit 1
> should work.
>
> When individual steps could fail and when I wanted to proceed
> executing the script to the end, I usually wrote a function that
> checked for failure status and only reported / generated a failure at
> the end.

Something like this:

run_cmd_nonfatal() {
  text="$1"
  shift
  if ! "$@"; then
    failures="$failures\n  - $text"
    return 1
  fi
  return 0
}

run_cmd_fatal() {
  text="$1"
  shift
  if ! "$@"; then
    echo "ERROR: $text"
    exit 1
  fi
  return 0
}

run_cmd_fatal "run portindex" portindex

But I don't know what exactly you want to do when an individual step fails.

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
In reply to this post by Mojca Miklavec-2
On Sat, Jul 01, 2017 at 09:21:42AM +0200, Mojca Miklavec wrote:
>I would say that
>    portindex || exit 1
>should work.

Does this make a difference? `portindex` returns 0 (success) even if
parse error occurred.

>When individual steps could fail and when I wanted to proceed
>executing the script to the end, I usually wrote a function that
>checked for failure status and only reported / generated a failure at
>the end. But I guess you can safely exit if portindex fails
>(potentially add some explicit sentence saying so if it's not clear
>from the error message).

If portindex couldn't parse the Portfile, I'd say we shouldn't waste
Travis's resource on the PR.

Another problem is that if our master branch contains broken Portfiles
(committed between PR parent commit and Travis build), should the build
fail and warn us or should we filter the irrelevant errors?

>Mojca

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Mojca Miklavec-2
On 1 July 2017 at 10:06, Zero King <[hidden email]> wrote:
> On Sat, Jul 01, 2017 at 09:21:42AM +0200, Mojca Miklavec wrote:
>>
>> I would say that
>>    portindex || exit 1
>> should work.
>
> Does this make a difference? `portindex` returns 0 (success) even if
> parse error occurred.

I'm sorry, I didn't know that. I only assumed that a non-zero value
wouldn't prevent the bootstrap script to continue and exit with 0 at
the end.

In that case portindex should be fixed as a high priority and return
non-zero value when it fails. (I still didn't test it).

I guess nobody really cared about this so far because commits should
generally never generate a parse error.

>> When individual steps could fail and when I wanted to proceed
>> executing the script to the end, I usually wrote a function that
>> checked for failure status and only reported / generated a failure at
>> the end. But I guess you can safely exit if portindex fails
>> (potentially add some explicit sentence saying so if it's not clear
>> from the error message).
>
> If portindex couldn't parse the Portfile, I'd say we shouldn't waste
> Travis's resource on the PR.

True. I wouldn't start the build anyway, I just wasn't sure whether
you wanted to continue in some other way.

> Another problem is that if our master branch contains broken Portfiles
> (committed between PR parent commit and Travis build), should the build
> fail and warn us or should we filter the irrelevant errors?

This is a completely separate "can of worms" :)
We should actually decide whether to build from master or from the
point where the commit was branched. It can be that some dependencies
were upgraded in the meantime (after PR was branched). Should the
build use the new or the old dependencies then?

One could argue that new ones make more sense because that's where the
changes will eventually end up. But then this makes non-deterministic
builds. Travis could launch three builds, some of them with
dependencies already updated and some with the old ones.

Or someone could commit something broken that gets fixed a few minutes
later, but the travis build would start building from the broken
commit and report a broken PR for no good reason.

I would be slightly in favour of taking the parent of PR as a
reference (that is: the exact state of the branch of the PR, so not
even trying to create portindex from master) unless that unnecessarily
complicates matters (if so, we can simply take trunk and stop worrying
about anything; in most cases that won't make a difference anyway).

What exactly did you have in mind with filtering?

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
On Sat, Jul 01, 2017 at 10:55:36AM +0200, Mojca Miklavec wrote:
>I would be slightly in favour of taking the parent of PR as a
>reference (that is: the exact state of the branch of the PR, so not
>even trying to create portindex from master) unless that unnecessarily
>complicates matters (if so, we can simply take trunk and stop worrying
>about anything; in most cases that won't make a difference anyway).

Travis only fetches the merged ref
`git fetch origin +refs/pull/542/merge:`, so it does complicate matters.

>What exactly did you have in mind with filtering?

I thought of `grep`ing the output for "Failed to parse file" and ignore
broken Portfiles not touched by the PR like this one
>Failed to parse file python/py-pydot/Portfile: can't read "_name": no such variable
in https://travis-ci.org/macports/macports-ports/jobs/248896726.

>Mojca

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Clemens Lang-2
Hi,

On Sat, Jul 01, 2017 at 10:37:04AM +0000, Zero King wrote:
> I thought of `grep`ing the output for "Failed to parse file" and
> ignore broken Portfiles not touched by the PR like this one
> > Failed to parse file python/py-pydot/Portfile: can't read "_name": no such variable
> in https://travis-ci.org/macports/macports-ports/jobs/248896726.

Rather than parsing the output of portindex, could we just run

  'port info'

in the directory of the modified portfile? That would catch the parse
error as well.

Changing portindex to return non-zero on parse error may not be what we
want. As a user, I don't want my portindex to fail (and potentially
abort further processing) if somebody accidentially commits a broken
Portfile that I don't care about.

--
Clemens
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Mojca Miklavec-2
On 1 July 2017 at 18:51, Clemens Lang wrote:
> On Sat, Jul 01, 2017 at 10:37:04AM +0000, Zero King wrote:

>> Travis only fetches the merged ref
>> `git fetch origin +refs/pull/542/merge:`, so it does complicate matters.

Isn't that exactly what we want?

>> I thought of `grep`ing the output for "Failed to parse file" and
>> ignore broken Portfiles not touched by the PR like this one
>> > Failed to parse file python/py-pydot/Portfile: can't read "_name": no such variable
>> in https://travis-ci.org/macports/macports-ports/jobs/248896726.

Is this the correct link?

> Rather than parsing the output of portindex, could we just run
>
>   'port info'
>
> in the directory of the modified portfile? That would catch the parse
> error as well.

If commit changed 20 directories (hopefully nobody submits another PR
like the one to remove $Id lines in a couple of thousand ports :), the
command would have to be executed in each and every one of them.

I simply cannot decide which approach is less "hacky", but anything
that works ...

> Changing portindex to return non-zero on parse error may not be what we
> want. As a user, I don't want my portindex to fail (and potentially
> abort further processing) if somebody accidentially commits a broken
> Portfile that I don't care about.

OK, that's a somewhat valid point. Maybe something like "portindex
--strict" would be suitable then :) Something that would return
non-zero value the :) :) :)
Just brainstorming, don't take my proposal too seriously.

I would say that this is not something that we should spend too much
time on for unrelated ports. I don't care if the complete build fails
if another unrelated port was in a bad shape. It may be that the
broken port is one of dependencies. The committer could have run
"portindex" himself, or in any case can act upon the problem once
discovered pretty quickly.

What is really important is to fail the build when portindex fails on
the submitted port. The use case that triggered this discussion was an
example where I could perhaps easily push the changes if it was
another much less relevant port submitted by maintainer. (I didn't see
the problem when first inspecting the changes. If maintainer
presumably tested it and the build succeeded ...)

Mojca
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
On Sat, Jul 01, 2017 at 10:53:41PM +0200, Mojca Miklavec wrote:
>On 1 July 2017 at 18:51, Clemens Lang wrote:
>> On Sat, Jul 01, 2017 at 10:37:04AM +0000, Zero King wrote:
>
>>> Travis only fetches the merged ref
>>> `git fetch origin +refs/pull/542/merge:`, so it does complicate matters.
>
>Isn't that exactly what we want?

Except when it's a fast-forward merge, Travis would not fetch the
commits in the PR. It only fetches the merge commit and some more
commits in master.

>>> I thought of `grep`ing the output for "Failed to parse file" and
>>> ignore broken Portfiles not touched by the PR like this one
>>> > Failed to parse file python/py-pydot/Portfile: can't read "_name": no such variable
>>> in https://travis-ci.org/macports/macports-ports/jobs/248896726.
>
>Is this the correct link?

Yes. Two ports failed and one of them is on our master branch.

>> Rather than parsing the output of portindex, could we just run
>>
>>   'port info'
>>
>> in the directory of the modified portfile? That would catch the parse
>> error as well.
>
>If commit changed 20 directories (hopefully nobody submits another PR
>like the one to remove $Id lines in a couple of thousand ports :), the
>command would have to be executed in each and every one of them.
It could be executed once with all ports changed as arguments, but
`portindex` is called before downloading CI bot and its dependencies.
I'd rather kill a build early if it contains broken Portfiles.

>I simply cannot decide which approach is less "hacky", but anything
>that works ...
>
>> Changing portindex to return non-zero on parse error may not be what we
>> want. As a user, I don't want my portindex to fail (and potentially
>> abort further processing) if somebody accidentially commits a broken
>> Portfile that I don't care about.
>
>OK, that's a somewhat valid point. Maybe something like "portindex
>--strict" would be suitable then :) Something that would return
>non-zero value the :) :) :)
>Just brainstorming, don't take my proposal too seriously.
We can patch the file on CI and leave it as it in -base.
Please review https://github.com/macports/macports-ports/pull/550.

>I would say that this is not something that we should spend too much
>time on for unrelated ports. I don't care if the complete build fails
>if another unrelated port was in a bad shape. It may be that the
>broken port is one of dependencies. The committer could have run
>"portindex" himself, or in any case can act upon the problem once
>discovered pretty quickly.
>
>What is really important is to fail the build when portindex fails on
>the submitted port. The use case that triggered this discussion was an
>example where I could perhaps easily push the changes if it was
>another much less relevant port submitted by maintainer. (I didn't see
>the problem when first inspecting the changes. If maintainer
>presumably tested it and the build succeeded ...)
The question is should I spend some time on filtering out irrelevant
broken ports. The important thing is done in PR#550.

>Mojca

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Travis: failed build reported as successful

Zero King-2
On Sun, Jul 02, 2017 at 02:41:55AM +0000, Zero King wrote:
>Except when it's a fast-forward merge, Travis would not fetch the
>commits in the PR. It only fetches the merge commit and some more
>commits in master.

Except for a fast-forward merge, ...

--
Best regards,
Zero King

Don't trust the From address.

smime.p7s (4K) Download Attachment
Loading...