I just posted a follow-up to our bug that old SWIG versions are not properly detected, leading to unclear failure messages: --- Actually, this just got a lot simpler. We used to support both 1.3.25+ and 1.3.24-, so we had to do both kinds of detection. Now, we REQUIRE 1.3.29, and therefore any SWIG that can''t give us its version number the new way is automatically too old. We can get rid of swigver.rb entirely. --- As with wx version detection, this can be done by someone who knows only ruby--no C++ or SWIG knowledge is required. Step right up, folks! You too can contribute to the wxruby project! Thanks, Kevin
Robin Stocker
2006-Aug-24 13:23 UTC
[Wxruby-users] Looking for volunteers (no C++ knowledge needed!)
Kevin Smith wrote:> As with wx version detection, this can be done by someone who knows only > ruby--no C++ or SWIG knowledge is required. Step right up, folks! You > too can contribute to the wxruby project!Hi, Thanks for the chance to contribute, even if it''s such a small thing :) Find attached the patch for rake/rakewx.rb. The file swig/swigver.rb isn''t needed anymore and it can be removed from CVS. I wasn''t sure whether to indent with tabs or spaces because there were both in rakewx.rb, so I just stuck with the standard Ruby style of using two spaces. I also took the liberty of changing the error message and simplifying some things. Hope you like it, Robin -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: swig-version-check.patch Url: http://rubyforge.org/pipermail/wxruby-users/attachments/20060824/3a3f01ca/attachment.pl
Kevin Smith
2006-Aug-24 21:00 UTC
[Wxruby-users] Swig version detection improvements (was: Looking for volunteers)
On Thu, 2006-08-24 at 15:23 +0200, Robin Stocker wrote:> Thanks for the chance to contribute, even if it''s such a small thing :)Small things can be really important. And every small thing I don''t have to do frees me up to do more of the "big stuff". So thanks!> Find attached the patch for rake/rakewx.rb. The file swig/swigver.rb > isn''t needed anymore and it can be removed from CVS.The patch doesn''t apply cleanly for me. It seems to have been generated from the wrong directory level. I worked around that, but have a couple other concerns about it. Can you double-check the "how to submit a patch" material on the wiki and recently posted to this list?> + unless version > + return false > + endI''m not a big fan of the "unless" keyword (again, I view it as a wart inherited from perl). Could this be if !version instead?> + ENV[''SWIGVER''] = versionI know this was in the existing code, but it raised a yellow flag for me. A bit of research shows that the only places it is used are swig/fixmodule.rb and fixmainmodule.rb. In both of those, there is a test for == 1.3.29, which seems wrong. It should be >= 1.3.29...except that now we ONLY support 1.3.29, so it looks like those if''s should be removed, and the code inside should always be executed. The comment should remain...and we probably should have an end comment to indicate that the comment applies to that whole series of transformations. If we take those out, then we really don''t need to set SWIGVER at all. If we really want to know the SWIG version, we can just look at the #define SWIGVERSION in each generated .cpp file. So, if it''s not too much to ask, could you fix those too, and re-submit this patch (also changing the "unless" and generating it from the correct directory level)? Thanks again, Kevin
Kevin Smith wrote:> In both of those, there is a test for == 1.3.29, which seems wrong. It > should be >= 1.3.29...except that now we ONLY support 1.3.29, so it > looks like those if''s should be removed, and the code inside should > always be executed. The comment should remain...and we probably should > have an end comment to indicate that the comment applies to that whole > series of transformations. > >This is on purpose to work around a bug in 1.3.29 that should be fixed in 1.3.30. In fact, I''ve compiled 1.3.30 so I can double-check the bug is fixed. Roy
On Thu, 2006-08-24 at 17:11 -0400, Roy Sutton wrote:> This is on purpose to work around a bug in 1.3.29 that should be fixed > in 1.3.30. In fact, I''ve compiled 1.3.30 so I can double-check the bug > is fixed.Ah. I didn''t realize 1.3.30 was done enough to be testable like that. I would still prefer that the test be done against the SWIGVER found in the .cpp file itself, rather than using an ENV variable to communicate which SWIG version we found. I can try to do it if you don''t have time, but I do consider it somewhat important. After the preview release would be fine, I suppose. So for the swigver redo, we should, for now, continue to set SWIGVER, and we should not modify fixmodule and fixmainmodule. Thanks, Kevin
Kevin Smith wrote: > On Thu, 2006-08-24 at 15:23 +0200, Robin Stocker wrote:> The patch doesn''t apply cleanly for me. It seems to have been generated > from the wrong directory level. I worked around that, but have a couple > other concerns about it. Can you double-check the "how to submit a > patch" material on the wiki and recently posted to this list?I''m sorry, I didn''t realise that I wasn''t in the top directory. I read the "how to" and saw that you use the diff -b option, so I changed the indenting to 4 spaces this time, to be a bit more consistent with the rest of the file.> I''m not a big fan of the "unless" keyword (again, I view it as a wart > inherited from perl). Could this be if !version instead?Well, I like it, although I sometimes use "if not" and sometimes "unless", depending on the probability of the condition being true :). Now I changed it to "if not", I hope this is also ok for you.> In both of those, there is a test for == 1.3.29, which seems wrong. It > should be >= 1.3.29...except that now we ONLY support 1.3.29, so it > looks like those if''s should be removed, and the code inside should > always be executed. The comment should remain...and we probably should > have an end comment to indicate that the comment applies to that whole > series of transformations.I saw the reply from Roy and you, so this isn''t included in the new patch. Regards, Robin -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: swig-version-check-2.patch Url: http://rubyforge.org/pipermail/wxruby-users/attachments/20060825/3d7ba806/attachment.pl
This patch looked great and has been committed. Thanks! Kevin On Fri, 2006-08-25 at 00:06 +0200, Robin Stocker wrote:> Kevin Smith wrote: > > On Thu, 2006-08-24 at 15:23 +0200, Robin Stocker wrote: > > > The patch doesn''t apply cleanly for me. It seems to have been generated > > from the wrong directory level. I worked around that, but have a couple > > other concerns about it. Can you double-check the "how to submit a > > patch" material on the wiki and recently posted to this list? > > I''m sorry, I didn''t realise that I wasn''t in the top directory. I read > the "how to" and saw that you use the diff -b option, so I changed the > indenting to 4 spaces this time, to be a bit more consistent with the > rest of the file. > > > I''m not a big fan of the "unless" keyword (again, I view it as a wart > > inherited from perl). Could this be if !version instead? > > Well, I like it, although I sometimes use "if not" and sometimes > "unless", depending on the probability of the condition being true :). > Now I changed it to "if not", I hope this is also ok for you. > > > In both of those, there is a test for == 1.3.29, which seems wrong. It > > should be >= 1.3.29...except that now we ONLY support 1.3.29, so it > > looks like those if''s should be removed, and the code inside should > > always be executed. The comment should remain...and we probably should > > have an end comment to indicate that the comment applies to that whole > > series of transformations. > > I saw the reply from Roy and you, so this isn''t included in the new patch. > > Regards, > Robin