How to submit Perl patches?
It's about time I learnt to submit proper patches. In the past I've just emailed module authors spelling mistakes and code fixes. This isn't good for the authors as then they have to scan through and manually edit the modules themselves. Thus making it more time consuming for them, and less likely for them to apply your fix. The proper way to do with is by supplying a diff which can be patched into the module. I've been sending lots of typo fixes to Mark Stosberg recently as I've been reading up on CGI::Application. I've promised him the next ones I send will be proper patches. Although as it turns out, my first patch wont be for CGI::App typos.
This post is a continuation of my last post, which you'll need to follow for the example to work.
Let's go through the process for the bug I found in the CGI::Kwiki test suite when ran under Windows.
First let look at the current test file (C:\temp\CGI-Kwiki-0.18\t\test.t):-
It's about time I learnt to submit proper patches. In the past I've just emailed module authors spelling mistakes and code fixes. This isn't good for the authors as then they have to scan through and manually edit the modules themselves. Thus making it more time consuming for them, and less likely for them to apply your fix. The proper way to do with is by supplying a diff which can be patched into the module. I've been sending lots of typo fixes to Mark Stosberg recently as I've been reading up on CGI::Application. I've promised him the next ones I send will be proper patches. Although as it turns out, my first patch wont be for CGI::App typos.
This post is a continuation of my last post, which you'll need to follow for the example to work.
Let's go through the process for the bug I found in the CGI::Kwiki test suite when ran under Windows.
First let look at the current test file (C:\temp\CGI-Kwiki-0.18\t\test.t):-
use lib 'inc';
use Test::More;
use Cwd;
use File::Path;
plan(tests => 9);
my $cwd = cwd;
rmtree('t/kwiki');
ok(mkdir 't/kwiki', 0777);
ok(chdir 't/kwiki');
ok(system("PERL5LIB=../../blib/lib;../../blib/script/kwiki-install") == 0);
ok(-f 'config.yaml');
ok(-d 'database');
ok(-f 'database/HomePage');
ok(-f 'database/KwikiFormattingRules');
ok(-f 'database/KwikiHelpIndex');
ok(chdir $cwd);
I can see the problem starts on line 12 with the call to system.
ok(system("PERL5LIB=../../blib/lib;../../blib/script/kwiki-install") == 0);
On Windows it's done a little differently. First I'll make a backup of the current test.t file called test.t.bak.
Now I'm changing that line to:-
if ( ( $^O eq 'MSWin32' ) || defined( $ENV{'OS'} ) ) {
ok(system("SET PERL5LIB=../../blib/lib") == 0 && system('..\..\blib\script\kwiki-install') == 0);
}#if
else {
ok(system("PERL5LIB=../../blib/lib;../../blib/script/kwiki-install") == 0);
}#else
On Linux that single call to system has set the PERL5LIB environment variable and then executed the kwiki-install script. For windows I've had to break it up into 2 system calls, use SET for the environment variable and fixed the Windows file path \'s, hence ' and not " otherwise I'd have to use \\. Internally Perl with convert / to \ when on Windows, but when you are talking to the OS directing with system you need to use \.
I've ran "nmake test" to see that this is working and it is. Brill :)
So now I have the new working test.t file and the old test.t.bak file.
Time for diff
Luckily diff and patch come as a part of UnxUtils that I described how to download and setup in my previous post.
First I need to change the file names, as I don't want my patch to be for test.t.bak, but for the original test.t. So I've renamed test.t (that has my working code) to test.new and test.t.bak back to test.t. (I had to name my new file test.t earlier in order for "nmake test" to work).
Next I open a command prompt and navigate to the C:\temp\CGI-Kwiki-0.18\t folder.
Now I run the diff program with the -u option for "Unified format", this is the diff format that most patch programs want.
diff -u test.t test.t.new
It gives me the output:-
--- test.t Sat Jan 24 16:45:54 2009
+++ test.t.new Sat Jan 24 17:06:14 2009
@@ -9,7 +9,12 @@
rmtree('t/kwiki');
ok(mkdir 't/kwiki', 0777);
ok(chdir 't/kwiki');
-ok(system("PERL5LIB=../../blib/lib;../../blib/script/kwiki-install") == 0);
+if ( ( $^O eq 'MSWin32' ) || defined( $ENV{'OS'} ) ) {
+ ok(system("SET PERL5LIB=../../blib/lib") == 0 && system('..\..\blib\script\kwiki-install') == 0);
+}#if
+else {
+ ok(system("PERL5LIB=../../blib/lib;../../blib/script/kwiki-install") == 0);
+}#else
ok(-f 'config.yaml');
ok(-d 'database');
ok(-f 'database/HomePage');
You can see the line numbers and the - before the line that is removed, and the + before the lines that need to be added. Before and after there are 3 lines of matching text. These are used so the patch program can still apply the diff if the line numbers don't match up.
The output looks good to me, so I'm going to run the command again and save the output to a file called test.patch:-
diff -u test.t test.t.ew > test.patch
Almost there. Now that I have my diff that explains how to apply my patch to test.t I need to test that it actually does it's job properly.
Time for patch
Patch is the program that'll apply the diff. I'm going to use it now to check that my patch file works properly. I'll be using the -b option so that the original file is backed up before it's patched.
patch -b test.t test.patch
Seems to have worked. The backup file is named test.t.orig, checking test.t I can see the patch has been applied correctly. If I wanted to remove the patch I can by running the command above with the -R option instead of -b. Or of course I could just restore the test.t.orig file.
Right, final test.
cd \temp\CGI-Kwiki-0.18
nmake test
Brilliant! So I've found a bug, fixed it, created the patch, and tested the patch out. Now I just need to get the patch to the module author and hope he applies it...
DOH! Just check the RT (Request Tracker) for Kwiki, it appears someone posted this as a critical bug 4 years ago! Yikes!:-
http://rt.cpan.org/Public/Dist/Display.html?Name=CGI-Kwiki
http://rt.cpan.org/Public/Bug/Display.html?id=2550
Aghh well. They didn't seem to provide a patch then, maybe if I submit a new bug with my patch it'll get used.
Lyle
Leave a comment