search

avr-llvm / llvm

[MERGED UPSTREAM] AVR backend for the LLVM compiler library
220 stars 21 forks source link

Why was #604 accepted even though it caused the build to fail? #233

Closed leksak closed 7 years ago

leksak commented 7 years ago

Subsequent commits did not fix the issue and so the build has been failing for the last 14 days.

shepmaster commented 7 years ago

Could you clarify a bit? This newly-opened issue is only 233; there's quite a long way before we get to 604 issues or PRs!

the build has been failing for the last 14 days

That's not supremely unusual for this repository, as we know there's quite a bit of work to be done on many fronts.

leksak commented 7 years ago

Could you clarify a bit? This newly-opened issue is only 233; there's quite a long way before we get to 604 issues or PRs!

Absolutely, by #604 I was referring to the build ID from the project build history which is available here: https://travis-ci.org/avr-llvm/llvm/builds

build-history

To address the following,

the build has been failing for the last 14 days

That's not supremely unusual for this repository, as we know there's quite a bit of work to be done on many fronts.

I understand. I might be alone in this but I find it hard to contribute to a project that I am completely unfamiliar with when the build is failing.

shepmaster commented 7 years ago

Absolutely, by #604 I was referring to the build ID from the project build history

Gotcha. Usually, when I hear "accepted", I think of something more active, like a commit or a pull request. The Travis build numbers are basically arbitrary, so they aren't top-of-mind.

Why was #604 accepted even though it caused the build to fail?

This project is attempting to do two hard things concurrently:

  1. Follow LLVM master.
  2. Add support for a brand-new processor.

In this particular case, @dylanmckay updated our fork of the code to follow upstream. The work required to do that merge was probably quite intricate. However, something about that update caused a single test to fail. At that point, we are left with two choices:

  1. Continue trying to hammer on the problem in isolation
  2. Publish the code and hope others can help.

Could we have pushed a PR and waited until the test was addressed before merging? Absolutely. However, that does introduce some overhead to the process. Dylan is really the core person driving all the effort right now, so whatever workflow fits them best is fine by me.

In a bigger project with more contributors, then I think that structure is fundamental. Perhaps if you join the effort, we will get one step closer to that point.

I might be alone in this but I find it hard to contribute to a project that I am completely unfamiliar with when the build is failing.

Nope, certainly not alone! However, I'd suggest that a failing test is a nice arrow pointing to a section of code that could use some extra attention, so it might be a good place to start if you'd still like to contribute. I'm pretty sure that's how I started 😉

All that said, I'm going to close this as a duplicate of #232, which is dedicated to fixing the specific failure at hand.

dylanmckay commented 7 years ago

@leksak has a good point, there is only a single test failing but it's definitely going to dissuade users and contributors from using a project if it's got a big red 'failing' badge.

dylanmckay commented 7 years ago

I've marked the test as XFAIL for now so we can fix it in #232 .

The build should go green now.

leksak commented 7 years ago

Great, thank you very much!