wxruby users - Aug 2006 - Looking for volunteers (no C++ knowledge needed!)

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

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

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
> +  end
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?
> +  ENV[''SWIGVER''] = version
I 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