Saturday, February 28, 2015

Kyua turns parallel

After three months of intensive work on Kyua's executor Git branch, I am happy to announce that the new execution engine, whose crown feature is the ability to run test cases in parallel, has just landed in master and passes all self-tests!

You can head over to the commit message for more details on the merge, read the NEWS entries, and skim throught the history of the executor branch to understand how this feature has been built.

One caveat: the history will look surprisingly short for a project that has spanned over three months. The reason is that, in the executor branch, I have routinely been using git rebase -i master to build a reduced set of changes that tell the story behind the implementation without distracting commits of the kind "Fix this little bug I forgot about" here and there. An unfortunate side-effect of this is that the temporal history of the commits makes no sense, and also that all the work I've been doing is not properly accounted for in GitHub's nice activity graphs; oh well, I consider a sane semantical history more important than these tiny details.

Why is this work important? Why is running tests in parallel such a big deal?

First, because the computing industry has fully moved into multiprocessor systems and thus taking advantage of multiple cores is something that any piece of modern software should do. As a little fun fact, Kyua is now able to stress my Mac Mini when running tests, spinning its fans to the maximum; this did not happen at all before.

But second, and I would say more importantly, because many tests are not resource-hungry or the system resources they stress do not overlap the resources used by other tests. For example: a significant number of tests are penalized by disk and time delays in them, which in turn cause the whole test suite to run for much longer than it would otherwise. Parallelization allows these long-running but not-heavy tests to run without blocking forward progress.

Let me also add that a secondary goal of this work is to optimize the inner workings of Kyua by reducing the system call overhead. In particular, eliminating one fork(2) call from every test case has been an explicit design goal. This especially helps Kyua when running on OS X, as fork is particularly expensive on the Darwin kernel (yes, citation needed).

As a very unscientific example: running the Kyua, ATF, Lutok, and shtk test suites with the old sequential execution engine takes about 2 minutes and 15 seconds in my dual-core FreeBSD virtual machine running on a dual-core Mac Mini. With the new implementation, the total run time goes down to 1 minute and 3 seconds using a parallelism setting of 4. Pretty cool I would say, but your mileage may (will) vary!

Are we done yet?

The merge of the executor branch marks the beginning of a major restructuring of Kyua's internals. As things are today, only the kyua test command has been switched to using the new execution engine, and not fully: only the execution of the test's body and cleanup routines happen through the executor; listing of test cases still happens as it did before. Similarly, both kyua list and kyua debug still use the out-of-process, testers-based, sequential implementation.

Therefore, there is a bunch of tricky work left to be done: the old test case execution engine (the runner plus the out-of-process testers) need to be fully removed, which in turn means that their functionality has to first be integrated into the new executor; there is a need for a feature to explicitly mark test programs as "exclusive", which is a prerequisite for tests that modify system-wide settings (as is done in the FreeBSD test suite); and an important regression needs to be fixed.

So... if we are not done, why merge now?

Because the current code is complete enough as a first piece of the whole puzzle. Even if the implementation does not yet meet my personal quality standards, the behavior of the code is already 80% of the way to my goal of fully switching to the new execution backend. You, as an end user, care about the behavior (not so much about the implementation), so by doing the merge now you can already start taking advantage of the new parallel execution functionality.

Also, because I am tired of managing a relatively large set of commits with git rebase -i. At this point, the set of commits that build the executor provide a good foundation for the code and its design. From now on, any other improvements to this codebase, such as the addition of new features or the correction of the existing regressions, should be properly tracked in the Git history.

And lastly because, at the beginning of 2015, I set myself the personal goal of getting this code merged by the end of February... so I just made the deadline! Which reminds me I gotta plan how the year's timeline looks like to reach Kyua 1.0.

Can I try it?

Of course! Please do!

There is no release available yet, but you can obviously fetch the code from the GitHub project page and and build it on your own! If you do that, do not forget to set parallelism=4 (or some other value greater than 1) in your ~/.kyua/kyua.conf file to enable the new behavior.

In fact, I am not going to cut a new release just yet because some of the issues mentioned above are of the "release-blocking" severity and thus must be resolved first. What I am going to do, though, is file bugs for each known issue so that they can be properly tracked.

Have fun and please share any feedback you may have!

Monday, February 16, 2015

Unused parameters in C and C++

Today I would like to dive into the topic of unused parameters in C and C++: why they may happen and how to properly deal with them—because smart compilers will warn you about their presence should you enable -Wunused-parameter or -Wextra, and even error out if you are brave enough to use -Werror.

Why may unused parameters appear?

You would think that unused parameters should never exist: if the parameter is not necessary as an input, it should not be there in the first place! That's a pretty good argument, but it does not hold when polymorphism enters the picture: if you want to have different implementations of a single API, such API will have to provide, on input, a superset of all the data required by all the possible implementations.

The obvious case of the above is having an abstract method implemented by more than one subclass (which you can think of as a function pointer within a struct in the case of C). In this scenario, the caller of this abstract method may be handling a generic condition but the various specific implementations may or may not use all the input data.

Our example

An example taken straight from Kyua is the compute_result method, whose purpose is to determine the status of a test case after termination based on the outputs of the test program, including: the program's exit code, its standard output, its standard error, and files that may be left in the transient work directory. The signature of this abstract method looks like this:

virtual model::test_result compute_result(
    const optional< process::status >& status,
    const fs::path& work_directory,
    const fs::path& stdout_path,
    const fs::path& stderr_path) const = 0;

Kyua implements this interface three times: one for plain test programs, one for ATF-based test programs, and one for TAP-compliant test programs. This interface receives all test-related post-termination data as inputs so that the different implementations can examine any parts (possibly not all) they require to compute the result.

In concrete terms: the plain interface only looks at the exit status; the ATF interface looks both at the exit status and at a file that is left in the work directory; and the TAP interface looks both at the exit status and the standard output of the program.

When you face an scenario like this where you have a generic method, it is clear that your code will end up with functions that receive some parameters that they do not need to use. This is alright. However, as obvious as it may be to you, the compiler does not know that and therefore assumes a coding error, warning you along the way. Not helpful.

Two simple but unsuitable alternatives

A first mechanism around this, which only works in C++, is to omit the parameter name in the function definition. Unfortunately, doing so means you cannot reference the parameter by name any longer in your documentation and, furthermore, this solution does not work for C.

A second mechanism is to introduce side-effect free statements in your code of the form (void)unused_argument_name;. Doing this is extremely ugly (for starters, you have to remember to keep such statements in sync with reality) and I fear is not guaranteed to silence the compiler—because, well, the compiler will spot a spurious statement and could warn about it as well.

Because these two solutions are suboptimal, I am not going to invest any more time on them. Fortunately, there is a third alternative.

Tagging unused parameters with compiler attributes

The third and best mechanism around this is to explicitly tag the unused parameters with the __attribute__((unused)) GCC extension as follows:

model::test_result compute_result(
    const optional< process::status >& status,
    const fs::path& work_directory __attribute__((unused)),
    const fs::path& stdout_path __attribute__((unused)),
    const fs::path& stderr_path __attribute__((unused))) const;

But this, as shown, is not portable. How can you make it so?

Making the code portable

If you want your code to work portably across compilers, then you have to go a bit further because the __attribute__ decorators are not standard. The most basic abstraction macro you'd think of is as follows:

#define UTILS_UNUSED __attribute__((unused))

... which you could parameterize as:

#define UTILS_UNUSED @ATTRIBUTE_UNUSED@

... so that your configure.ac script could determine what the right mechanism to mark a value as unused in your platform is and perform the replacement. This is not trivial, so take a look at Kyua's compiler-features.m4 for configure.ac to get some ideas.

Such a simple macro then lets you write:

model::test_result compute_result(
    const optional< process::status >& status,
    const fs::path& work_directory UTILS_UNUSED,
    const fs::path& stdout_path UTILS_UNUSED,
    const fs::path& stderr_path UTILS_UNUSED) const;

... which gets us most of the way there, but not fully.

Going further

The UTILS_UNUSED macro shown above lets the compiler know that the argument may be unused and that this is acceptable. Unfortunately, if an argument is marked as unused but it is actually used, the compiler will not tell you about it. Such a thing can happen once you modify the code months down the road and forget to modify the function signature. If this happens, it is a recipe for obscure issues, if only because you will confuse other programmers when they read the code and cannot really understand the intent behind the attribute declaration.

My trick to fix this, which I've been using successfully for various years, is to define a macro that also wraps the argument name; say: UTILS_UNUSED_PARAM(stdout_path). This macro does two things: first, it abstracts the definition of the attribute so that configure may strip it out if the attribute is not supported by the underlying compiler; and, second and more importantly, it renames the given argument by prefixing it with the unused_ string. This renaming is where the beauty lies: the name change will forbid you from using the parameter via its given name and thus, whenever you have to start using the parameter, you will very well know to remove the macro from the function definition. Has worked every single time since!

Here is how the macro looks like (straight from Kyua's defs.hpp.in file):

#define UTILS_UNUSED @ATTRIBUTE_UNUSED@
#define UTILS_UNUSED_PARAM(name) unused_ ## name UTILS_UNUSED

And here is how the macro would be used in our example above:

/// This is a Doxygen-style docstring.
///
/// Note how, in this comment, we must refer to our unused
/// parameters via their modified name.  This also spills to our
/// public API documentation, making it crystal-clear to the
/// reader that these parameters are not used.  Because we are
/// documenting here a specific implementation of the API and not
/// its abstract signature, it is reasonable to tell such details
/// to the user.
///
/// \param status Status of the exit process.
/// \param unused_work_directory An unused parameter!
/// \param unused_stdout_path Another unused parameter!
/// \param unused_stderr_path Yet another unused parameter!
///
/// \return The computed test result.
model::test_result compute_result(
    const optional< process::status >& status,
    const fs::path& UTILS_UNUSED_PARAM(work_directory),
    const fs::path& UTILS_UNUSED_PARAM(stdout_path),
    const fs::path& UTILS_UNUSED_PARAM(stderr_path)) const;

What about Doxygen?

As I just mentioned Doxygen above, there is one extra trick to get our macros working during the documentation extraction phase. Because Doxygen does not implement a full-blown C/C++ parser—although I wish it did, and nowadays this is relatively easy thanks to LLVM!—you have to tell Doxygen how to interpret the macro. Do so with the following code to the Doxyfile control file:

PREDEFINED  = "UTILS_UNUSED="
PREDEFINED += "UTILS_UNUSED_PARAM(name)=unused_ ## name"

So, what about you? Do you keep your code warning-free by applying similar techniques?

Thursday, February 05, 2015

One month in with Android Wear

Or: A review of the LG G Watch.

Right before the Christmas holidays, I was gifted an LG G Watch Black Titan, a relatively simple smartwatch that sports the new Android Wear operating system:

After over a month of daily use, I am now comfortable about writing about my impressions. But, before doing that, let me set this review in the right context.

Context-setting

Sometime last summer, the continuous interruptions from my phone ended up irritating me significantly. It seems like pretty much any app you install these days insists on notifying you of their super-important stuff at any time (reminds me of the beginning of the Windows 95 era and the poison that all the apps that registered themselves in the taskbar notification area were—or still are). In an attempt to minimize the annoyances without going back to a dumbphone (which I actually considered!), I ended up doing two things: first, disabling as many of the notifications as possible; and, second, starting to wear a regular watch again to minimize the times I compulsively looked at my phone just to check the time. You know, checking the time is enough to suck you into looking at random online content if there are any pending notifications you did not know about.

As you can imagine knowing this, when I received the smartwatch my first thought was that I would not like it: I would not tolerate being bothered by more intrusive, "on-your-face" notifications. That said, there was one feature compelling enough to convince me of giving it a try: Strava—the sports-tracking app that I use to record bike routes and runs—had a plug-in for Android Wear, and the ability to have quick access to my statistics in the middle of an activity sounded rather cool.

Also, let's face it: having been given a shiny new toy, I could really not resist trying out the LG G Watch so I started wearing it exclusively to have first-hand (haha, pun intended) knowledge of what this new technology has to offer. Spoiler alert: I confess that I am enjoying this experience more than I originally envisioned. Let's see why.

Poor battery life... not?

The first thing people like to mock about these new smartwatches is the fact that they need to be charged every day. Yes, so what? I have yet to find that to be a problem.

For starters, the battery lasts a whole day just fine. If I wear and use the watch on those long days that span from 8am to 10pm, the watch still has, easily, 50% battery left if not more. I don't see it "dying" with normal use during a regular day. Even then, should you forget to charge the watch overnight, you can give it a quick boost during your morning home routine and that will probably charge it enough to survive throughout your day.

Those facts overcome the worry of running out of batter midday and being left with a silly brick on your wrist, but you will still need to charge the watch every day. That said... I don't know about you, but I cannot sleep wearing a watch any longer. So, instead of just dropping the watch on the nightstand, take the little extra effort to let it rest on its dock. Boom. Fresh battery next morning ready to chew through the day.

Dorky looks maybe?

That smartwatches look ugly is the second most common mockery about them. But what can I say? The LG G Watch is just one of the many available smartwatches; if you don't like it, there are plenty other options to suit your taste. However, it is true that the smartwatches are still rather thick and, in general, bulky today—but this could be said of any new technology (remember the first cellphones? the first laptops?). Give smartwatch manufacturers a few years and, if the concept sticks, I am sure we will all see sleeker watches that rival the appearance of the fanciest analog watches.

But I'm not going to lie to you: it is true that the LG G Watch, in particular, is pretty big. In fact, it looks out of place on thin wrists. But, once again, that's a concern about a specific model and not about the product concept in general.

Watch and phone pairing

Android Wear relies on an Android smartphone, paired via a Bluetooth connection, for all of its Internet-based functionality. If the phone is off or out of reach, your watch is not able to do anything... other than tell you the time which, you know, is just fine considering that we are talking about a watch first and foremost here!

Now, when the watch is in reach of the phone, something interesting becomes available: the watch can be set up as a trusted Bluetooth device, so when the two are in reach, the phone will remain unlocked and not ask you for any PIN or pattern on wake-up. (Did you know you can do the same with your Bluetooth-enabled car?) While this is quite convenient, I have mixed feelings if only because my 2-year old daughter is now able to mess around with the phone at will.

Notifications: not so intrusive after all!

The most basic thing (after telling you the time) that an Android Wear watch will do for you is display the notifications you get on your phone no matter what app they come from—even if that app is not Wear-aware yet. You can dismiss these notifications trivially with a swipe-style gesture and you can interact with them to perform related actions. For example: if you get notified of an incoming WhatsApp message, you can either see the new text or scroll through the history of messages; or if you receive an incoming message, you can reply to it using voice dictation.

The deal breaker for me is being notified of incoming phone calls via the watch thanks to a smooth and very recognizable vibration pattern. I do not miss phone calls any more, and that's something certain family members (my wife, that is) are already happy about. (I'm known to not notice that my phone is ringing even when I am clearly hearing the sound and wondering aloud what it might be; true story, just happened this last weekend. Again.)

Control your phone

The way I generally see Android Wear at the moment is as a remote control for the phone. Notifications are just one aspect of it, but the watch can be used to command the phone to do a variety of things, including: opening a web search, starting the recording of an activity, taking a note, dialing a phone number, etc. The majority of these actions can be triggered both with touch or by voice, but some require voice interaction because the watch has no input mechanism for text.

The exciting part of this "remote control" functionality is that it can be extended by third-party apps. The Android Wear Face API provides the mechanism by which Android apps can expose extra functionality through the watch. The Strava app that I mentioned above is one clear example: the watch suddenly gains actions like "start a run" or "start a ride" and, when activated, the watch displays the current pace and lets you control the ongoing activity without having to touch the phone. Same goes, say, for Spotify, and probably for any other app in the next months to come.

Summing it up

All in all, I am positively surprised about these new smartwatches. This product is something I would have never bought for myself, and I confess I was skeptical about them at first. But I decided to give them a try with an open mind and it has already been over a month of continuous use with no intentions of leaving it behind. I like the watch: it has given me additional freedom from my phone and I am sure not to miss important phone calls any longer!

Give smartwatches the benefit of the doubt and, if you have the chance, use one for a few days. You may be positively surprised.

Thursday, November 20, 2014

Task tracking and the Bullet Journal

iGTD, Things, Org mode, OmniFocus, Google Tasks, Trello, Google Keep... All of them. All of them I have tried over the last few years and all of them have failed me in some way — or, rather, I have failed to adjust to their idiosyncrasies. Part of it is because the overwhelming feeling that builds up after days of continuous use due to how easy it is to end up with never-ending piles of open tasks.

In other words: even after making the effort to adjust to these systems, none of them have really worked for me. The one that kept me on track for the longest was Org mode, topping at three months of continuous use plus some more months of occasional use, which is nothing for a system that is supposed to help you plan your day, week, month, year and, well, life.

But about nine months ago, I made a discovery. I stumbled upon the Bullet Journal: a simple yet effective system to track tasks using traditional pen and paper. After watching the introductory video and giving the system a test drive for a week on an old notebook, I set out to invest a few extra dollars into a nicer journal:

Nice notebooks are "pricey," and that's why I bought one: the point of the exercise was to trick myself into using the shiny new purchase... and the trick worked for long enough to hook me into the system. Not surprisingly though: previous experiments of mine at tracking tasks with a simple and unstructured sheet of paper resulted in productivity boosts and a feeling of accomplishment that I did not get otherwise.

What is the Bullet Journal?

The Bullet Journal is an extremely simple notebook-based task-tracking system designed by Ryder Carroll. At its core is something called Rapid Logging, a set of trivial rules to record, in a chronological manner, tasks, notes, and events. The system adds some more structure to this by dividing the chronological list of items into days or topics, and by outlining a monthly spread and task management workflow.

It is interesting to see how such simple concepts can work so well in practice and, especially, how popular this system has become over the last couple of years. Just look at the Google+ community behind this methodology and the success of Ryder's Kickstarter project to see what I mean.

Monthly review: the best part of it

The monthly review is the part of the Bullet Journal system that I enjoy the most and the part that truly makes me feel the usefulness of the system. 9 months and 100 pages in and I can reflect and see that this system has made a positive difference in my work.

If you have ever read David Allen's Getting Things Done, you know that regular reviews are essential to ensure that the contents of your system are up-to-date and remain relevant. While it is easy (for me) to overlook this important thinking time with a computerized system, it is pretty much impossible to do so with a notebook system because of its linearity: when the next month arrives, the preparation of the new monthly spread triggers the review of the previous month.

That's, precisely, why I like it. According to the Bullet Journal guidelines, you should consolidate all of your pending tasks from the last month into a "carryover section" in the current month. Because the system is chronological and paper based, the implication is that this consolidation involves manually copying tasks from previous pages into the new month. Sounds burdensome? Exactly, and that's why this works so well: having to copy a task over will push you to decide whether the task is still relevant, whether the effort to do it at that point is lower than copying the task, or whether the task can be moved to the calendar because it now has a deterministic start/due date.

This consolidation step into a new monthly spread is also critical in keeping your journal manageable. Because all tasks are consolidated into a new month, you are left with a reduced set of pages to go through to locate open tasks. In my case, I only use about 10–15 pages per month, which means that this is the amount of pages I have to review on a daily or weekly basis to see where my projects are at.

My own tweaks: habits and weekly planning

Another great aspect of the Bullet Journal—or really, any journal—is that it is free form: if you don't like the rules, just come up with your own. To that extent, there are plenty of ideas online in places such as the aforementioned Google+ community, a multitude of blog posts, and at some point the new web-based portal that Ryder is working on.

Personally, I have adopted two tweaks inspired by what other people were doing.

The first is the incorporation of monthly and weekly goal-setting sections. These take the form of lists of coarse-grained tasks representing goals, and these lists serve as the basis from which to derive finer-grained tasks when planning the current day. These tasks representing goals cannot be easily accomplished, but that's by design: the idea is to use them to distill more specific "next actions" while at the same time having some kind of status record of your week and month.

The second is the tracking of habits. In my monthly spread, I reserve the right page to track a bunch of habits I want to build. I do so by matching up various columns to the calendar days and then just crossing out the tasks on the days when they are done. These include things like reading, cooking, shaving, or commuting by bike.

So far, I have only gotten so-so results from these two tweaks. Especially, the upfront planning that is necessary to make goal-setting work is a habit in itself that needs to be built, but an important one as plenty of productivity books will teach you. (See Decide by Steve McClatchy or The One Thing by Gary Keller.) When I spend time planning ahead, the following days are much more productive; however, without the ingrained habit, it very easy to get side-tracked and end up not doing any such planning.

The major drawback: future planning

The one thing I haven't really yet figured out is managing long-term planning with the Bullet Journal methodology.

An idea that comes to mind involves keeping individual pages to jot down project milestones and week-to-week expectations, but I'm not sure that would be easy to deal with: somehow I personally need the ability to visualize the timeline of the project on a calendar with all the time impediments that may exist. In this regard, I have been happy at work by depicting the available time as a collection of weeks and drafting the expected status of each project on those future weeks.

Also, the calendar spread proposed by the Bullet Journal is of limited value to me. I am very used to the online Google Calendar for timed appointments, and I am now abusing it to also mark tasks that need to be started at some point in the far future. For example, I now trust the Calendar system to remind me, five years from now, to renew my passport.

Again, the system is so free form that you can play with your own ideas on how to manage your tasks. Whatever suits best your workflow, go for it; and if that means combining the journal with an electronic system, so be it!

Summing it up

To this day, I am still surprised what a difference pen and paper has made on my productivity compared to computerized task tracking systems.

Sure, I sometimes miss the ability to access my system anytime, anywhere. But having a system that is always-on is useless if the system is forgotten. Carrying a notebook around has forced me to think about what I write in it and to want to look at it later on.

Give it a try. Do so by visiting the Bullet Journal website and watching the video.

Monday, November 17, 2014

shtk 1.6 is now available

The Shell Toolkit, or shtk for short, is a little project I introduced back in August of 2008 to support other tools such as sysbuild and sysupgrade. Since that time, the project has seen little activity because it did not have much to offer and because shtk's public interface was not documented (hence making it impossible for developers to get started with shtk).

Well, both are changing today with the brand-new release of shtk 1.6:

The first major change is the addition of manual pages to document the public API of shtk. After installation, type man 3 shtk to open the shtk(3) reference manual and dive straight into a comprehensive set of documentation. Pay special attention to the SEE ALSO sections and follow the links form there. Also note that there is an individual manual page for each public function offered by shtk so it's trivial to access the documentation from your favorite editor when you highlight a symbol name.

And why manual pages? Because shtk is designed to fit the minimalist user environment of a typical BSD system. The reason shtk exists in the first place is because such systems don't have any other high-level language available for scripting, yet scripting languages are ideal for system administration tools.

The second major change is the addition of a shiny-new unit-testing library, known as unittest, which I briefly described in a post to the shtk-discuss mailing list. This library is on-par, functionality-wise, with atf-sh. In fact, unittest is purely written in the shell scripting language thus avoiding the dependency on C++ that atf-sh has. Not to mention that the codebase is more modern and easier to extend.

If you have never used shtk before, I invite you to test-drive it now that we have fully-fledged documentation and some compelling functionality to offer!

Enjoy the ride.

Saturday, May 31, 2014

Code review culture meets FreeBSD

One of the things that often shocks new engineers at Google is the fact that every change to the source tree must be reviewed before commit. It is hard to internalize such a workflow if you have never been exposed to it, but given enough time —O(weeks) is my estimation—, the formal pre-commit code review process becomes a habit and, soon after, something you take for granted.

To me, code reviews have become invaluable and, actually, I feel "naked" when I work on open source projects where this process is not standard practice. This is especially the case when developing my own, 1-person projects, because there is nobody to bounce my code off for a quick sanity-check. Fortunately, this may not be the case any more in, at least, FreeBSD, and I am super-happy to see change happening.

A few individuals in the FreeBSD project have set up an instance of Phabricator, an open source code review system, that is reachable at http://reviews.freebsd.org/ and ready to be used by FreeBSD committers. Instructions and guidelines are in the new CodeReview wiki page.

To the FreeBSD committer:

Even if you are skeptical —I was, back when I joined Google in 2009— I'd like to strongly encourage you to test this workflow for any change you want to commit, be it a one-liner or a multipage patch. Don't be shy: get your code up there and encourage specific reviewers to comment the hell out of it. There is nothing to be ashamed of when (not if) your change receives dozens of comments! (But it is embarrassing to receive the comments post-commit, isn't it?)

Beware of the process though. There are several caveats to keep in mind if you want to keep your sanity and that's what started this post. My views on this are detailed below.

Note that the Phabricator setup for FreeBSD is experimental and has not yet been blessed by core. There is also no policy requiring reviews to be made via this tool nor reviews to be made at all. However, I'm hopeful that things may change given enough time.


Let's discuss code reviews per se.

Getting into the habits of the code review process, and not getting mad at it, takes time and a lot of patience. Having gone through thousands of code reviews and performed hundreds of them over the last 5 years, here come my own thoughts on this whole thing.

First of all, why go through the hassle?

Simply put: to get a second and fresh pair of eyes go over your change. Code reviews exist to give someone else a chance to catch bugs in your code; to question your implementation in places where things could be done differently; to make sure your design is easy to read and understand (because they will have to understand it to do a review!); and to point out style inconsistencies.

All of these are beneficial for any kind of patch, be it the seemingly-trivial one-line change to the implementation of a brand-new subsystem. Mind you: I've seen reviews of the former class receive comments that spotted major flaws in the apparently-trivial change being made.

The annoying "cool down" period

All articles out there providing advice on becoming a better writer seem to agree on one thing: you must step away from your composition after finishing the first draft, preferably for hours, before copyediting it. As it turns out, the exact same thing applies to code.

But it's hard to step aside from your code once it is building and running and all that is left for the world to witness your creation is to "commit" it to the tree. But you know what? In the vast majority of cases, nobody cares if you commit your change at that exact moment, or tomorrow, or the week after. It may be hard to hear this, but that pre-commit "high" that rushes you to submit your code is usually counterproductive and dangerous. (Been there, done that, and ended up having to fix the commit soon after for stupid reasons... countless times... and that is shameful.)

What amuses me the most are the times when I've been coding for one-two hours straight, cleaned up the code in preparation for submission, written some tests... only to take a bathroom break and realize, in less than five minutes, that the path I had been taking was completely flawed. Stepping aside helps and that's why obvious problems in the code magically manifest to you soon after you hit "commit", requiring a second immediate followup commit to correct them.

Where am I going with all this? Simple: an interesting side-effect of pre-commit reviews is that they force you to step aside from your code; they force you to cool down and thus they allow you to experience the benefits of the disconnect when you get back to your change later on.

Keep multiple reviews open at once

So cooling down may be great, but I hear you cry that it will slow down your development because you will be stuck waiting for approval on a dependent change.

First of all, ask yourself: are you intending to write crappy code in a rush or, alternatively, do you care about getting the code as close to perfect as possible? Because if you are in the former camp, you should probably change your attitude or avoid contributing to a project other people care about; and if you are in the latter camp, you will eventually understand that asking for a review and waiting for your reviewer to get back to you is a reasonable thing to do.

But it is true that code reviews slow you down unless you change your work habits. How? Keep multiple work paths open. Whenever you are waiting for a change to be reviewed, do something else: prepare a dependent commit; write documentation or a blog post; work on a different feature; work on a completely-separate project; etc. In my case at work, I often have 2-3 pending changes at various stages of the review process and 1-2 changes still in the works. It indeed takes some getting used to, but the increased quality of the resulting code pays off.

Learn to embrace comments

Experienced programmers that have not been exposed to a code review culture may get personally offended when their patches are returned to them with more than zero comments. You must understand that you are not perfect (you knew that) and that the comments are being made to ensure you produce the best change possible.

Your reviewers are not there to annoy you: they are there to ensure your code meets good quality standards, that no obvious (and not-so-obvious) bugs sneak in and that it can be easily read. Try to see it this way and accept the feedback. Remember: in a technical setting, reviewers comment on your ideas and code, not on you as a person — it is important to learn to distantiate yourself from your ideas so that you can objectively assess them.

I guarantee you that you will become a better programmer and team player if you learn to deal well with reviews even when it seems that every single line you touched receives a comment.

Selecting your reviewers

Ah... the tricky part of this whole thing, which is only made worse in the volunteer-based world of open source.

Some background first: because code reviews at Google are a prerequisite for code submission, you must always find a reviewer for your change. This is easy in small team-local projects, but with the very large codebase that we deal with, it not always is: the original authors of the code you are modifying, who usually are the best reviewers, may not be available any longer. FreeBSD also has a huge codebase, older than Google's, so the same problem exists. Ergo, how do you find the reviewer?

Your first choice, again, is to try and find the owner of the code you are modifying. The owner (or owners) may still be the original author if he is still around, but it can be anyone else that stepped in since to maintain that piece of code.

Finding an individual owner may not possible: maybe the code is abandoned; maybe it is actively used but no single individual can be considered the owner. This is unfortunate but is a reality in open source. So do you abandon the code review process?

Of course NOT! Get someone with relevant expertise in the change you are making to look at your code; maybe they won't be able to predict all of the consequences of the change, but their review is lightyears better than nothing. At work, I may "abuse" specific local teammates that I know are thorough in their assessment.

The last thing to consider when selecting your reviewers is: how picky are they? As you go through reviews, you will learn that some reviewers will nitpick every single detail (e.g. "missing dot at end of sentence", "add a blank line here") while others will only glance over the logic of the code and give you a quick approval. Do not actively avoid the former camp; in fact, try to get them involved when your primary reviewer is on the latter; otherwise, it's certain that you will commit trivial mistakes (if only typos). I'm in the nitpickers camp and proudly so, if you ask.

Should all of the above fail, leaving you without a reviewer: ask for volunteers! There will probably be someone ready to step in.

Set a deadline

Because most committers in open source projects are volunteers, you cannot send out a change for review and wait indefinitely until somebody looks. Unless you are forbidden to commit to a specific part of the tree without review, set a deadline for when you will submit the change even if there have been no reviews. After all, the pre-commit review workflow in FreeBSD is not enforced (yet?).

If you end up committing the change after the deadline without having received a review, make sure to mention so in the commit message and clearly open the door at fixing any issues post-commit.

Learn to say no

Because code reviews happen in the open, anybody is allowed to join the review of a patch and add comments. You should not see this as an annoyance but you must know when to say no and you must clearly know who your actual approvers are and who are just making "advisory" comments.

Also note that comments in a review are not always about pointing obviously-wrong stuff out. Many times, the comments will be in the form of questions asking why you did something in a specific way and not another. In those cases, the comment is intended to start a discussion, not to force you to change something immediately. And in very few cases, the discussion might degenerate in a back-and-forth against two very valid alternatives. If this happens... you'll either have to push your way through (not recommended) or find a neutral and experienced third reviewer that can break the deal.

Get to the reviews!

Wow, that was way longer than I thought. If you are interested in getting your code for FreeBSD reviewed — and who wouldn't be when we are building a production-quality OS? — read the CodeReview wiki page instructions and start today.

And if you have already started, mind to share your point of view? Any questions?

Friday, May 23, 2014

Refocusing Kyua maybe?

The FreeBSD devsummit that just passed by gave me enough insight into Jenkins to question the long-term plans for Kyua. Uh, WHAT?! Let me explain.

In the beginning...

One of the original but unstated goals of Kyua was to fix the "mess" that is the NetBSD releng test run logs site: if you pay close attention, you will notice that various individuals have reinvented the wheel over and over again in an attempt to automate release builds and test suite runs. In other words: different parties have implemented independent continuous integration systems several times with more or less success.

Ending up with such duplication of infrastructure was never an intended result of ATF as ATF should have had this functionality on its own. Unfortunately, my lack of experience on continuous integration when I started ATF seven years ago made ATF's plans and design not cover the truly end goal of having a system up and running for all the platforms we care about.

In other words: even if this was never published on the Kyua website, my long-term plan was to turn Kyua into a continuous integration platform. This would involve providing some form of automation to execute builds along the execution of tests, a mechanism to distribute build and test tasks to more than one machine, a comprehensive web interface to interact with the system, a monitoring system to ensure the platform keeps up and running, and much more.

As you can imagine, implementing all of these features is a humongous amount of work and Kyua is currently far from providing any of these. (Note: it is relatively simple to come up with a simple do-one-thing implementation, but productizing and productionizing the result is a completely different story — and I am not here to ship toy-quality software!)

Enter Jenkins

During the May 2014 FreeBSD devsummit, I got to learn about Jenkins: An extendable open source continuous integration server. Thanks to Craig Rodrigues, I had a local instance up and running in less than 20 minutes and was able to hack Kyua to output Jenkins-compatible reports in less than an hour of hands-on work. (Sorry, changes still not pushed to Github.)

As it turns out, Jenkins is no joke. It is a hugely popular piece of software used by high-profile projects and companies. Just think on holding a conference on such a niche tool and getting 400+ attendees to understand this.

My original reaction to the UI was to dismiss it as ugly (it indeed is), but a bit of poking revealed a large amount of hidden-but-easily-reachable functionality. And after visiting the FreeBSD Jenkins deployment, I quickly realized the power of this system once I saw it distributing jobs across a cluster and monitoring their status in real time and in a relatively nice way.

What a discovery. Jenkins does all I ever wanted Kyua to do and more, much more. "Fortunately," Jenkins lacks the pieces to actually define and execute the FreeBSD/NetBSD test suite, and this is exactly what Kyua offers today — which means that Kyua is not irrelevant.

Not enough time

So we have Jenkins out there that does most of what I ever wanted Kyua to do and we have Kyua which currently does not do any of it but fulfills the one missing piece in Jenkins. Hmm... sound like an opportunity to rethink Kyua's goals maybe. But why? It all revolves around time and spending it wisely in the things that will have the most impact with the fewer effort.

The time I can devote to Kyua is minimal these days, so at my current pace I —or, rather, we— will never reach those grandiose goals of having native continuous integration in Kyua. What's worse: the fact that I had such plans in mind but no time for them made me feel bad about Kyua overall (I'm pretty sure there is a name for this apathetic feeling but couldn't find it).

Learning about Jenkins has been a relief. "What if... what if I could reshape Kyua's goals to be less ambitious in scope? What if Kyua did one thing only, which would be to define and execute a test suite in a single configuration under a single machine, and excelled at it? Then you'd be free to plug Kyua into whichever continuous integration system that best suit your needs and Jenkins would be a reasonable choice." Those are the kinds of questions that are floating my mind since last week.

No downsides?

The main problem with Jenkins and the context in which we want to use it in (BSD systems) is, probably, the fact that Jenkins is written in Java. Java support on the BSDs has never been great, but things generally work OK under amd64 and i386. This actually is a serious issue considering all the platforms that FreeBSD and NetBSD support. Without going too far, I now would like to run Jenkins at home to offer continuous integration for Kyua itself... but my two servers are PowerPC-based machines — so no Jenkins for them yet, at least under FreeBSD.

Let us not get distracted from the core idea of splitting continuous integration out of Kyua just because Jenkins may not fit some obscure use case out there. Jenkins may be a very good solution to this, but it need not be the only one!

TL;DR

The more I think about it, the more I become convinced that Kyua's goals need to be simplified. Let Kyua focus on what it already is good at: defining a complex test suite and making sure that it can be run easily and deterministically. Let the remaining scaffolding for continuous integration to the big players out there. Jenkins, in this case, would be just one of the possible options for the continuous integration framework; we'd just need to make sure that Kyua plays well with at least one other system to ensure the design is generic.

If we went this route, Kyua would not need to be as complex as it already is. As a result, there are several things in the current design that could be yanked and/or streamlined to offer a simpler and more reliable piece of software.

It really all boils down to a very simple idea:

Optimize my little and precious time to have the greatest impact on the project and other people's work.

The alternative to this is to remain in the status quo, which is to naively wait for years until Kyua gets all of the necessary continuous integration features and makes Jenkins and other custom scripts unnecessary. And I cannot promise any of this will ever happen, nor I can say it makes sense at all.

I haven't made a decision yet as I would like these thoughts to solidly settle before announcing any changes widely, and I would also like to think about any possible simplifications to Kyua's design.

In the meantime: what are your thoughts?