Saturday, August 20, 2011

Cleaning patches for the review

Unfortunately (and, as usual, for historical reasons) we keep quite a few auto-generated files in wxWidgets sources repository, for example configure (created by autoconf), a lot of setup.h files all generated from the master setup_inc.h template using a simple sed script and also tons of makefiles (created by bakefile).

One of the many reasons this is inconvenient is that changes to these files get in the way when reviewing patches. But luckily, it's simple enough to fix this, at least if you're using a Unix system with patchutils on it. It's enough to execute this command:

filterdiff -x '*/*.vcproj' -x '*/*.dsp' -x configure -x \
'*/makefile.*' -x '*' -x \
-x include/wx/msw/wince/setup.h -x '*/setup0.h' \
some-wx-patch.diff > some-wx-patch-clean.diff

The command is actually not difficult to write but it is long and it's easy to forget something (as I just did for not the first time when retyping it) so I recorded it in a one-line script called clean_patch and now you (and I) can simply do

./misc/scripts/clean_patch some-wx-patch.diff > some-wx-patch-clean.diff

If you add any new files, any new wxUSE_XXX constants or modify, please remember to clean your patches up. The clean patches are much more pleasant to review and easier to merge, even if there were other changes to the auto-generated files in the meanwhile.


zooplah said...

OK, why not just get rid of these auto-generated files altogether? Most projects don't have those files in the repository. They have an file that generates them; they're in the tarballs, but you're expected to have the autotools and run configure if you checkout from the repository.

VZ said...

Personally I'd be all for removing them (notice the "unfortunately" starting this post) but other people object to having to install autoconf and bakefile before building wx from svn. I guess the real problem is that our releases are too infrequent and too many people use svn instead and they want to be able to do it as simply as possible. In any case, this is not that easy to fix as it's a "political" rather than technical problem.