On Sun, 2008-04-20 at 06:27 -0700, James Keenan via RT wrote: > 1. I tried your patch out. It did not apply cleanly at first, so I had > to do a little editing. I am attaching an 'svn diff' that represents > the changes actually made.
Yep, that was because someone else had committed a change before my patch was accepted. (I believe you figured that out on IRC already, this comment is for historical record.) > This patch worked for me on Darwin in the sense that it detected OpenGL > where previously the step class had not: > > Determining if your platform supports OpenGL............yes, MacOSX_GLUT 5. Excellent. > 2. Last night I wrote some tests for this step class along the lines of > those found for other configuration step classes, e.g., testing for > settings of results, testing for verbose output, etc. However, since > the internals of this class are still under development, I have > temporarily disabled some of those tests so that we don't get spurious > failures. Thank you for both making the tests, and disabling them until we get the code churn settled down a bit. :-) > 3. May I suggest that this code: > > $has_glut = _handle_glut($conf, $self->_evaluate_cc_run($test, > $verbose)); > > ... be rewritten like this: > > my $rv = $self->_evaluate_cc_run($test, $verbose); > $has_glut = _handle_glut($conf, $rv); > > This would be more consistent with the way other config step classes are > coded in that it gives the method call its own statement. Originally it was written in two-line fashion. I merged it into one line because I realized _handle_glut and _evaluate_cc_run were starting to have an API between them that the calling code didn't need to understand. Yes, that's a design smell, but late last night deciding not to try to purge the design smell was the difference between getting a fresh patch done and not. :-) > Thank you very much. And thank you; I'm very glad to get all the help I can with the porting issues -- and writing tests for it all is a nice bonus. :-) -'f
