45

One programmer committed some work to the SVN repository, then went home. After he left, the Hudson automatic build failed. Another programmer saw this, and after looking through the code changes, detected that the problem was the absence of one library. He added this library to SVN and the next build completed successfully.

Did the second programmer do the right thing or should he have just waited until the first programmer fixed the issue?

Kris
  • 304
  • 1
  • 7
nahab
  • 301
  • 3
  • 5
  • 31
    Question: One programmers member asked a question. Another member read the question and saw some syntactical and grammatical errors, so he decided to edit the question and correct them, to make the question a little bit easier to read. Is what the editor did right or should he had just waited for the poster to fix the errors? – yannis Nov 08 '11 at 11:17
  • 2
    What are your team rules for this situation? –  Nov 08 '11 at 11:23
  • @YannisRizos. This is absolutely difficult situations. In this case my English not so good and I cannot correct myself because I don't understand where is the problem. We can say that I am "unprofessional" in English. But if developer is unprofessional in java then no one will be correct his code after each his commit. He will be just fired.
    I asked about cases when developer forgot commit some changes etc.
    – nahab Nov 08 '11 at 12:03
  • 4
    @nahab Oh, don't worry, I'm not saying that it's a problem :). Just that in a community, as in a team, members helping each other should be encouraged. Also I don't think that a developer breaking a build is unprofessional, even if for a minor bug, these things happen to the best of us. – yannis Nov 08 '11 at 12:10
  • 11
    The whole idea of _having_ Hudson in the first place is because humans are humans and will break the build once in a while. You just want to catch it early. It could be argued that the programmer in question should have verified that the build built before going home. –  Nov 08 '11 at 12:40
  • 2
    On our team, we review the build result, mention it to the responsible party, and fix it if it's easy. The only people who don't make mistakes are the ones who aren't working. – Mike Dunlavey Nov 08 '11 at 13:49
  • 1
    @ThorbjørnRavnAndersen: But he didn't, so arguing over that point gets you nowhere. For easy stuff, just fix it. For complex stuff, roll it back (or quarantine it on a branch, depending on SCM in use). – Donal Fellows Nov 08 '11 at 14:34
  • 2
    As mentioned, it's good teamwork to fix the build. And yes, the original dev should have checked to make sure it built prior to check-in, but everyone makes mistakes. There's a million good reasons an otherwise good programmer and person, who would normally take the time to check his work got rushed and simply forgot, or didn't have the time to wait for the build. You fix it, fire the guy a polite email so they know what you did, not for blame, but perhaps the intention was to remove the library and a spot was missed and the fix wasn't just re-adding it... – CaffGeek Nov 08 '11 at 14:35
  • @Donal, that would be a good candidate for a new office rule. Do not go home until your check-in has been built successfully. –  Nov 08 '11 at 14:47
  • 2
    @Thorbjørn, sure but shit happens and we shouldn't pretend otherwise. Step 1 is always to fix it by the most expedient method as that's the best for the rest of the team. Arguing over the _post mortem_ is far less useful and an abrogation of collective responsibility. – Donal Fellows Nov 08 '11 at 15:03
  • @Donal hence the "_new_ rule". Of course accidents happen, but checking in without having time to await the check, is one of the habits you do not encourage people to have. –  Nov 08 '11 at 15:19
  • See also: [Should I tell someone that their commit caused a regression?](http://programmers.stackexchange.com/q/110987/20011) – Caleb Nov 08 '11 at 15:36
  • 14
    This is much more easily comprehended if you consider the opposite--If the build is broken, slowing down the entire team (even at home, after hours) and you can fix it but make a deliberate choice not to due to some point of procedure, should you be allowed to keep your job? – Bill K Nov 08 '11 at 17:01
  • @ThorbjørnRavnAndersen: Not a bad rule, but there *will* be exceptions to it (for example, if Hudson (or Jenkins) has stopped working). – Keith Thompson Nov 08 '11 at 21:26
  • @BillK, inverting the question helps, but the answer is still "it depends." That point of procedure may be there for a very good reason; violating it could lead to immediate dismissal. The cost of leaving the build broken overnight might be low while the cost of making a mistake while fixing the problem could be high. A broken build is usually somewhere between an inconvenience and a hassle, and an easy fix is usually safe enough, but we just don't have that information here. – Caleb Nov 08 '11 at 22:33
  • 1
    @Caleb If that's the case I would seriously consider trying to convince my group to check in smaller chunks--a few a day at least--or is the problem that what you consider a "Build" actually deploys which is even more scary. Fixing a broken build should be pretty easy and often involves just fixing a broken test case someone forgot to run, but leaving a build broken piles up (it's already broken, I can't trust which tests are broken and which I might have broken--I'll just check in) – Bill K Nov 09 '11 at 01:33
  • 1
    Having the build green should be a Team comittment so that as soon as the build is broken, there isn't more urgent task than just fixing it. And spreading such a culture in the team is the best way to avoid the [commit & run](http://97things.oreilly.com/wiki/index.php/Commit-and-run_is_a_crime.) syndrome. You don't want to impact your peers so you do your best to check in proper stuff. – pierroz Nov 09 '11 at 09:44
  • The question shouldn't be whether what the second programmer did was right, but rather how the second programmer should approach the subject with the first programmer to avoid it happening again... If this can be avoided in future, then this was a "one-off" and the question of right/wrong is eliminated on the grounds that the programmer made a judgement call in an unusual situation – m-smith Nov 09 '11 at 16:02

16 Answers16

87

It depends to some extent on how your team usually works, but I would say that was fine. Keeping the build working saves everyone else time.

It's polite for the the second programmer to drop the first an email to explain what he has done, just in case a specific version of the library is needed or there is some other complication. It's also a slightly more subtle way to point out that they had broken the build.

Luke Graham
  • 2,393
  • 18
  • 20
12

It depends.

  • Is the bug so obvious that adding a library is the way to fix it? Sometimes the fix is in finding a workaround not to need that library.

  • Is the project in a phase where all changes must be linked to an existing ticket? If so, did you file a ticket? Has that ticket been assigned to you?

Anyway, focus on fixing the bug, not on blaming the responsible.

mouviciel
  • 15,473
  • 1
  • 37
  • 64
11

Yes it's okay. However, it's unprofessional for the original programmer to go home before testing wether the build would compile.

Your reputation is 100% in your control. Stuff like this tarnishes your reputation and trying to polish a tarnished reputation is very difficult.

  • 2
    +1 for putting the onus on the first developer to test the build. The second paragraph really isn't true or relevant. Other people can damage your reputation, either intentionally or not, even when your behavior is completely above-board. – Caleb Nov 08 '11 at 14:17
  • 6
    It is entirely possible that the original programmer had the library on his machine, but the machine doing the auto build didn't. Yes, the library should be in SVN, but this can be a really subtle problem to not even notice. – mpdonadio Nov 08 '11 at 14:49
7

Communicate

There is no strict rules (beside your own team rules) for those scenario.

Dev2 should be able to tell dev1 he can fix his error, neither one of them should fear something resulting from this exchange, they are part of a team.

xsace
  • 695
  • 4
  • 15
5

Why not? If your product is more important than fixing blames, it is certainly all right. Although a build failing because of library change is pretty lame and you need to reprimand the developer for not testing it.

DPD
  • 3,527
  • 2
  • 16
  • 22
3

Build failures happen. If it's important that a daily build happen then I would fix it and then request that the developer that checked in the broken code to review the fix the next day and ensure that the code is now as it should be.

As has been said, the guy who fixed it should probably email the guy who broke it and detail what the fix was.

AndrewC
  • 349
  • 1
  • 4
2

My motto, is don't commit to SVN after 3pm that way you can always fix your own build failures.

If you do not fix his/her build failure, then everybody else's build will also fail. I would fix it to save time in the long run, but make sure they are aware that you had to fix it.

Having some sort of 'point the finger of blame' script is a good way to do this, or make the person who breaks the build buy donuts!!

  • 2
    Our CI tool actually has an option to send e-mail to the developer who broke the build (in addition to the rest of the team). – TMN Nov 08 '11 at 14:33
2

Someone needs to fix it and the first programmer should not have gone home without first making sure that he had not broken the build. However, for such an easily fixed problem, calling him back to fix it himself would be extreme.

I agree with Luke Graham's suggestion of sending an explanatory e-mail, although I'd say it's more than polite - it's basic communication.

  • With integration builds sometimes taking an hour or more depending on the complexity of your system, you'd have to implement a "commit cutoff" every day to ensure that the last build of the day will occur while everyone's still around. Even then, people have doctor appointments, kids' soccer practice, etc. and need to duck out immediately, regardless of build status. Agile says that work should be at a sustainable pace and should not be a drain on the workers. Keeping them there till 8:00 to watch a build succeed is contrary to that. – KeithS Nov 08 '11 at 14:55
  • @KeithS: True. But I've found that, regardless of when I leave, the most likely time for me to break the build is when I'm in a hurry: right before lunch, right before a meeting, right before the end of the day. So I think it's a "personal best practice" to not commit anything when there isn't enough time to watch and fix the build afterward. – Daniel Pryden Nov 08 '11 at 16:19
2

Yes yes yes! It fosters collective code ownership and sets a kind of healthy peer-pressure in the team to keep a high standard and not let a broken window scenario develop. A bit of communication to let the other developer know is a good idea.

Peter Kelly
  • 263
  • 1
  • 6
2

I think it's OK to fix obvious things - i.e., if you are 100% sure the guy whose code you're fixing would make the same - or substantially the same - fix. If the fix is more complex, it is usually polite to talk to the person whose code you're fixing - it may be that you misunderstood the intent or the reason for breakage is not what you thought it is, or maybe he intended another fix but for some reason couldn't commit it just yet (life happens, you know :).

In general, the rule usually is: you break the build - you fix the build, but there are exceptions, especially if the fix is obvious and/or the person responsible is unreachable.

Of course, if you have the case of serial build breaker - especially with the pattern "checked in, went home, build broken for days" - the responsible person needs to get some talking to about why CI systems and tests exist and how one should check before checking in :)

StasM
  • 3,377
  • 2
  • 23
  • 28
1

Things happen. The failure to add a new code file (whether source or compiled) to Subversion is probably the most common cause of broken builds, assuming it worked on the developer's computer. At my last job with a CI environment, even the most senior guys sometimes forgot.

I think, if another person was able to fix the build and thus keep the team humming along, that's fine. I do think the programmer who went home needs at least a friendly e-mail stating what happened, and to remind him to make sure that new code gets added before commits. If it happens often, maybe make that a minor offense punishable by the "dance of shame", to help reduce occurrences (and lighten the mood).

KeithS
  • 21,994
  • 6
  • 52
  • 79
1

It depends on Team dynamics, but in an ideal world everyone on the Team would "own" the whole project, all of the code, and consequently, all of the bugs jointly. So if you find a problem you fix it, and communicate with the originator of the bug only if there's some specific added value to the code in doing so.

Dave
  • 869
  • 5
  • 7
0

It's OK to fix unless this is a regular occurance in which case I would have the boss call him and make him return and fix it himself.

HLGEM
  • 28,709
  • 4
  • 67
  • 116
0

It depends, it depends...

As programmers, our work is to get things working, not to judge people. So I'd say that the best thing you can do is to fix it, or if it is not obvious, just roll back the changes and let the first programmer know so he can fix it later.

Anyway, having the latest guy that broke the build to wear a weird hat is enough to pay more attention the next time ^_^

fortran
  • 458
  • 2
  • 10
0

In some environments, this is very rude, and for good reasons. In other environments, it's expected, and for good reasons.

In still other environments, it's very rude or expected for very bad reasons.

It largely depends how critical a broken build is versus how critical a verified correct build. And to some extent, it depends on how obvious it was that the fix was the right fix and the only one needed.

David Schwartz
  • 4,676
  • 22
  • 26
0

First, 'went home' is an anachronism. Programmers don't go home anymore -- they are just either online or offline. You could ping and wait.

More seriously, there are actually two parts to the question. 'looking through the code changes' is fine; rest may not be the right thing to do. What if his judgement of a missing library was wrong?

Kris
  • 304
  • 1
  • 7