Bug #4214: Fiddle::WINDOWS == false on Windows

• Status: Closed
• Assignee: tenderlovemaking (Aaron Patterson)
• Target version: 1.9.2
• ruby -v: ruby 1.9.2p136 (2010-12-25 revision 30363) [i386-mingw32]

[jonforums (Jon Forums)]

=begin

 
 C:\Users\Jon>ruby --version
 ruby 1.9.2p136 (2010-12-25 revision 30363) [i386-mingw32]
 
 C:\Users\Jon>ruby -e "require 'fiddle';puts Fiddle::WINDOWS"
 false
 
 

The root cause was that configure.in never checked for windows.h. Consequently, on Windows, .ext/include/i386-mingw32/ruby/config.h never defined the HAVE_WINDOWS_H macro used by fiddle in ext/fiddle/{fiddle.c,fiddle.h,function.c}. FYI, the HAVE_WINDOWS_H macro is also used by ext/dl/{cfunc.c,dl.h,handle.c} but I've not tested the DL code on Windows. Fiddle and DL appear to be the only places in the code that HAVE_WINDOWS_H is checked.

After the attached patch on trunk@30404 I get

 
 C:\Users\Jon>ruby --version
 ruby 1.9.3dev (2010-12-27 trunk 30404) [i386-mingw32]
 
 C:\Users\Jon>ruby -e "require 'fiddle';puts Fiddle::WINDOWS"
 true
 
 

The patch was tested on (a) Windows 7 Ultimate 32-bit using the RubyInstaller build recipes, (b) Arch Linux 2.6.36 32-bit, and (c) Arch Linux 32-bit cross-compiled for Windows. Running @make test-all TESTS=fiddle@ passes on both Win7 and Arch. The portion of the patch relating to win32/Makefile.sub is speculative as I don't build with VC. Perhaps Park H. or the mswin32 maintainer will verify.

Please backport an acceptable version of the patch to ruby_1_9_2 at your earliest convenience.

Jon
=end

[luislavena (Luis Lavena)]

=begin
Perhaps Fiddle can match what DL is already doing:

in ext/dl/extconf.rb

have_header("windows.h")

=end

[jonforums (Jon Forums)]

=begin
While it might be nice in the future to have HAVE_WINDOWS_H globally available in config.h, currently it's not used anywhere else in the codebase. Maybe the unit test is the only thing that survives ;)
=end

[tenderlovemaking (Aaron Patterson)]

=begin
On Tue, Dec 28, 2010 at 01:06:00AM +0900, Luis Lavena wrote:

Issue #4214 has been updated by Luis Lavena.

Perhaps Fiddle can match what DL is already doing:

in ext/dl/extconf.rb

have_header("windows.h")

Yes, I would rather do this.

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
This issue was solved with changeset r30407.
Jon, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

[luislavena (Luis Lavena)]

=begin
Thank you Jon and Aaron for applying.

Created backport #4216 for Ruby 1.9.2 maintainer.
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Tue, 28 Dec 2010 01:24:29 +0900,
Jon Forums wrote in [ruby-core:33925]:

While it might be nice in the future to have HAVE_WINDOWS_H globally
available in config.h, currently it's not used anywhere else in the
codebase. Maybe the unit test is the only thing that survives ;)

Why not using WIN32 predefined macro?

--
Nobu Nakada

=end

[tenderlovemaking (Aaron Patterson)]

=begin
On Tue, Dec 28, 2010 at 07:49:07AM +0900, Nobuyoshi Nakada wrote:

Hi,

At Tue, 28 Dec 2010 01:24:29 +0900,
Jon Forums wrote in [ruby-core:33925]:

While it might be nice in the future to have HAVE_WINDOWS_H globally
available in config.h, currently it's not used anywhere else in the
codebase. Maybe the unit test is the only thing that survives ;)

Why not using WIN32 predefined macro?

Is this better than checking for windows.h? If so, I'd rather use it
than checking for some header file.

I'm away from home for a while, so I don't have access to my windows
machine to test.

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

[jonforums (Jon Forums)]

• File predefs.c predefs.c added

=begin
If you go the predefined macro route, better use _WIN32 rather than WIN32 as both cl [1] and MinGW gcc define it. Using the attached simple test...

c:\Users\Jon\Documents\CDev\sandbox>cl /h
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 16.00.30319.01 for 80x86

c:\Users\Jon\Documents\CDev\sandbox>predefs.exe
_WIN32 defined

Now MinGW gcc...

C:\Users\Jon\Documents\CDev\sandbox>gcc --version
gcc (tdm-1) 4.5.1

C:\Users\Jon\Documents\CDev\sandbox>predefs.exe
WIN32 defined
_WIN32 defined
MINGW32 defined

[1] http://msdn.microsoft.com/en-us/library/b0084kay.aspx
=end

[luislavena (Luis Lavena)]

=begin
Nobu, something like this could work for you?

diff --git a/ext/fiddle/extconf.rb b/ext/fiddle/extconf.rb
index 3dcd914..03b0ac2 100644
--- a/ext/fiddle/extconf.rb
+++ b/ext/fiddle/extconf.rb
@@ -18,7 +18,6 @@ unless have_library('ffi') || have_library('libffi')
end

have_header 'sys/mman.h'
-have_header 'windows.h'

create_makefile 'fiddle'

diff --git a/ext/fiddle/fiddle.c b/ext/fiddle/fiddle.c
index 78e21c5..2580ac0 100644
--- a/ext/fiddle/fiddle.c
+++ b/ext/fiddle/fiddle.c
@@ -18,7 +18,7 @@ void Init_fiddle()
rb_define_const(mFiddle, "TYPE_FLOAT", INT2NUM(TYPE_FLOAT));
rb_define_const(mFiddle, "TYPE_DOUBLE", INT2NUM(TYPE_DOUBLE));

-#if defined(HAVE_WINDOWS_H)
+#if defined(_WIN32)
rb_define_const(mFiddle, "WINDOWS", Qtrue);
#else
rb_define_const(mFiddle, "WINDOWS", Qfalse);

===

There is no windows.h check and we rely on _WIN32 that is defined either if you're on 32 or 64 bits.
(above patch is against trunk codebase)

Cheers.
=end

[tenderlovemaking (Aaron Patterson)]

=begin
On Wed, Dec 29, 2010 at 12:03:48AM +0900, Luis Lavena wrote:

Issue #4214 has been updated by Luis Lavena.

Nobu, something like this could work for you?

diff --git a/ext/fiddle/extconf.rb b/ext/fiddle/extconf.rb
index 3dcd914..03b0ac2 100644
--- a/ext/fiddle/extconf.rb
+++ b/ext/fiddle/extconf.rb
@@ -18,7 +18,6 @@ unless have_library('ffi') || have_library('libffi')
end

have_header 'sys/mman.h'
-have_header 'windows.h'

create_makefile 'fiddle'

diff --git a/ext/fiddle/fiddle.c b/ext/fiddle/fiddle.c
index 78e21c5..2580ac0 100644
--- a/ext/fiddle/fiddle.c
+++ b/ext/fiddle/fiddle.c
@@ -18,7 +18,7 @@ void Init_fiddle()
rb_define_const(mFiddle, "TYPE_FLOAT", INT2NUM(TYPE_FLOAT));
rb_define_const(mFiddle, "TYPE_DOUBLE", INT2NUM(TYPE_DOUBLE));

-#if defined(HAVE_WINDOWS_H)
+#if defined(_WIN32)
rb_define_const(mFiddle, "WINDOWS", Qtrue);
#else
rb_define_const(mFiddle, "WINDOWS", Qfalse);

===

There is no windows.h check and we rely on _WIN32 that is defined either if you're on 32 or 64 bits.
(above patch is against trunk codebase)

I prefer this solution. If you windows guys say it's good, then I think
we should apply. :-D

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

[luislavena (Luis Lavena)]

=begin
On Tue, Dec 28, 2010 at 5:00 PM, Aaron Patterson
[e-mail] wrote:

I prefer this solution.  If you windows guys say it's good, then I think
we should apply.  :-D

Then that is the solution :-)

We just need to correct the backport request to indicate the commit.

Thank you Aaron,

Luis Lavena
AREA 17

Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

=end

[jonforums (Jon Forums)]

=begin
I don't expect any surprises, but I want to try _WIN32 out on the following files and run test-all my Win7 and Arch systems and report back:

C:\Users\Jon\Documents\RubyDev\ruby-git>grep -rn HAVE_WINDOWS_H ext
ext/dl/cfunc.c:26:#if defined(HAVE_WINDOWS_H)
ext/dl/cfunc.c:581:#if defined(HAVE_WINDOWS_H)
ext/dl/cfunc.c:610:#if defined(HAVE_WINDOWS_H)
ext/dl/cfunc.c:616:#if defined(HAVE_WINDOWS_H)
ext/dl/dl.h:23:# if defined(HAVE_WINDOWS_H)
ext/dl/handle.c:10:#ifdef HAVE_WINDOWS_H
ext/dl/handle.c:145:#if defined(HAVE_WINDOWS_H)
ext/fiddle/fiddle.c:21:#if defined(HAVE_WINDOWS_H)
ext/fiddle/fiddle.h:7:#if defined(HAVE_WINDOWS_H)
ext/fiddle/function.c:128:#if defined(HAVE_WINDOWS_H)

Jon
=end

[tenderlovemaking (Aaron Patterson)]

=begin
On Wed, Dec 29, 2010 at 05:09:34AM +0900, Jon Forums wrote:

Issue #4214 has been updated by Jon Forums.

I don't expect any surprises, but I want to try _WIN32 out on the following files and run test-all my Win7 and Arch systems and report back:

Please let me know how it works out. I'll switch all of these checks to
_WIN32 if it works well. Deleting code is one of my favorite tasks!
:-D

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

[jonforums (Jon Forums)]

• File 0001-windows-macros-for-dl-n-fiddle.patch 0001-windows-macros-for-dl-n-fiddle.patch added

=begin
Attached patch gives the following results on Win7 Ultimate 32-bit with similar results on Arch Linux except on Arch the results were "123 tests, 229 assertions, 0 failures, 0 errors, 0 skips"

I didn't mod DL's extconf.rb as I've not looked over DL's code enough to feel comfortable doing so.

C:\projects\rubyinstaller-git\sandbox\ruby19_build>make test-all TESTS="fiddle dl"
./miniruby.exe -I../../../../Users/Jon/Documents/RubyDev/ruby-git/lib -I.ext/common ../../../../Users/Jon/Documents/RubyDev/ruby-git/tool/runruby.rb --extout=.ext
-- "../../../../Users/Jon/Documents/RubyDev/ruby-git/test/runner.rb" fiddle dl
Run options:

Running tests:

....................................................................S....SS..........S......................................

Finished tests in 0.126516s, 980.1132 tests/s, 1770.5270 assertions/s.

1. Skipped:
   test_sinf(DL::TestFunc) [C:/Users/Jon/Documents/RubyDev/ruby-git/test/dl/test_func.rb:29]:
   libm may not have sinf()

2. Skipped:
   test_DEFAULT(DL::TestHandle) [C:/Users/Jon/Documents/RubyDev/ruby-git/test/dl/test_handle.rb:169]:
   DL::Handle::DEFAULT is not supported

3. Skipped:
   test_NEXT(DL::TestHandle) [C:/Users/Jon/Documents/RubyDev/ruby-git/test/dl/test_handle.rb:136]:
   DL::Handle::NEXT is not supported

4. Skipped:
   test_static_sym(DL::TestHandle) [C:/Users/Jon/Documents/RubyDev/ruby-git/test/dl/test_handle.rb:25]:
   DL::Handle.sym is not supported

124 tests, 224 assertions, 0 failures, 0 errors, 4 skips
=end