15 Dez 2014

50503

0

On Patches

Before I begin, I should make clear that for all intents and purposes I am not a "developer" on the project. I have actually tried very hard to avoid falling into that role, as most people who do become developers end up getting sucked into that work to the point where of the time they do dedicate to the project, they tend to not have any left over to do other work that the project needs. The project generally relies on me to do some web content related work, moderate the forums, and a few administrative things here and there related to spinning out point releases. Were I to become a fully fledged developer my other work would likely get neglected, a fact that a good percentage of the team would find unpalatable, especially when it comes to forum moderation. That said, I do undergo brief bursts of productivity wherein I end up dealing with some work outside of my "official" remit. The past half-week was one such burst, precipitated by me doing some other work I am responsible for but that I had been putting off due to lack of time.

I rarely look at Jira, mainly because Jira is mostly for bugs in ReactOS and even reports related to "online matters usually fall outside of my area of responsibility. I maintain content after all and I do not have much if anything to do with the actual software of the site. There were however some old Jira issues related to content that I had effectively forgotten about and in the middle of last week I was reminded of their existence while dealing with another issue. After fixing a couple of them, and due to one of the IRC regulars having been badgering the devs to review his patches while I was in the channel, I got curious as to how badly we were doing in that department. I expected it to be pretty bad, since the project has a really crappy track record for responding to general tickets much less patches. I was not, disappointed, I suppose, when I found I believe 129 open tickets with patches attached. My level of irritation was somewhat elevated, to say the least. I began looking over the patches to try and see what they were about and quite a few were quite simple or low hanging fruit. That was about when my irritation peaked and I just started committing them.

The vast majority of the patches were by one Ricardo Hanke, who has been remarkably patient considering some of his patches were from a year ago. Most of his fixes tend to be attempts to provide a smoother user experience, plugging in functionality holes in bundled applications and fixing incorrect assumptions or missing data in others. Most of this was pretty low hanging fruit and should not have needed to wait this long to get reviewed, since reviewing them took all of maybe five to ten minutes. A few took longer, but the basic functionality was all pretty straightforward and did not require major effort. Another individual with quite a few outstanding patches is Lee Schroeder and his work is quite comparable to the type of work done by Hanke. I'm still working my way through Schroeder's stuff, but again the majority of his patches are fairly low hanging fruit that can go in with minimal fuss.

Not all patches are simple however, and there have been three somewhat big ones that I've pushed in, one which by all rights needed further design work but I'll get to that in a bit. The first "big" one was basically a partial rewrite of the comp utility, Windows' equivalent of the diff tool. The previous version assumed text files, while the patched version also performs a binary diff. This patch was from over three years ago and so at this point is the oldest patch I've shoved through. At this point I do not even know the name of the developer, just their user name of kruntuid, which is something of a shame as he or she certainly deserves to be credited better. It is however not the oldest issue that has drawn my attention, but more on that later. The second "big" one was an implementation of the tree utility, a command line program that prints out where in a drive's hierarchy a directory is. This was pretty well done for all intents and purposes, there were no major "glaring" issues beyond the developer needing to be more careful about conforming to the project's coding style. At the same time the program exposed the lack of the "secure" CRT functions in ReactOS' CRT exports, which was somewhat annoying. That's something that I will need to, discuss, with the rest of the team to figure out what the blazes is actually supposed to happen. And of course after the tree utility went in, Schroeder had a patch submitted the next day to make some improvements. The last of the "big" patches was to modify the task manager to prevent it from killing certain processes. Specifically, if you kill CSRSS, you will bugcheck the OS since CSRSS is basically the parent process for damn near everything and manages all of the services. This was an instance of where I jumped the gun, as apparently that was still ongoing discussion of what the best way to do this was. Of course, since the developers had not bothered to put an actual note in the issue mentioning that they were still trying to hash things out, I had no awareness of said discussion. So far as I can tell no one has bothered to actually revert the patch, and my response to the other developers was basically, "Start reviewing patches yourselves and actually comment on them if you want to avoid this happening again in the future." Yes, I am being an asshole about this, but times like this it's necessary.

A couple of big patches are still in my queue and they depend very much on me finding time to take a closer look due to their complexity. There's an old one in here too, first reported in 2010. The latest iteration of the patch is from 2013, but the age is still mildly irritating. The issue itself is adding the ability to use proxies to RosApps. Personally I really do not care about RosApps, since I always saw it as little more than a glorified download manager. It certainly is not a package manager, whatever people might want to believe. But it's still used by the community and fleshing out its basic functionality is important enough to warrant some attention, especially when someone else was willing to do the heavy lifting. The main issue is, aside from the amount of new code, is that the patch from 2013 is not applying entirely cleanly. Fortunately the problematic area is in the language resource files, but untangling it properly is going to take at least a little bit of time, time which I need to find. There's also at least a couple of other RosApp related patches that I might take the time to look at, but they're lower in the priority scale. Another big patch is actually by Hanke, and it's to switch the wallpaper rendering from using GDI to GDI+. Why does this matter? Well, for one it would allow for using images in formats besides BMP as wallpapers. If Thomas Faber doesn't get around to reviewing it by the time I do, well, I'll just go and take the ticket and do my stuff. It's also a very big patch, one that I will need to spend time understanding the code, not just scanning for basic problems or code style issues. Then there is a patch involving adding support for some printf related functionality. That one, that one looks nasty and very complicated and my only real qualification for looking at it is due to the fact that I've debugged printf related bugs in the past. To say I am not looking forward to trying to grapple with that patch would be an understatement. The last big patch that I am aware of and am inclined to take a look at is related to paint. There's apparently a problem with the selection boxes and there's a patch to fix it, but this one came into my radar very recently and I am still of two minds whether I can review it. We'll see.

In addition to the above, there are quite a few other one-liners that are in parts of the system that I do not know enough to review. For that matter, there are a couple of bigger patches that need their respective domain experts to examine. To this end, I've also been hounding/badgering a couple of developers to get them to start looking at patches again. Perhaps somewhat ironically, there is also an entire body of patches that were created by the developers themselves and they had posted them to get a second opinion from another developer. I am somewhat curious as to whether how many of them are still valid and whether some have been superseded due to more recent code churn.

At this point the count of outstanding issues with patches attached is down to 113. In truth that number has been fluctuating a bit because a couple of new patches have been submitted in the interim and I've closed a few issues that were not going to see anything committed because they were improper or had been superseded. Still, the number of patches waiting for review is far higher than I am personally happy with. There is unfortunately not much that can be done about this since reviewing patches, even the simplest ones, takes time. Time is not something the developers have in abundance and nothing I do is going to magically give them more time. The only way I can help to reduce the count is to review some myself. Those will mostly be in less critical areas of ReactOS, usually applications or control panel applets or if I am feeling especially brave lower level pieces where I have a firm understanding of what is going on. Still, over the course of the past week a good number of patches have been dealt with. Maybe by the end of next year the number will fall to below 100. One can hope anyway.

Discussion: https://reactos.org/forum/viewtopic.php?f=2&t=13864

This blog post represents the personal opinion of the author and is not representative of the position of the ReactOS Project.