Project

General

Profile

Bug #4214

Fiddle::WINDOWS == false on Windows

Added by jonforums (Jon Forums) about 9 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Target version:
ruby -v:
ruby 1.9.2p136 (2010-12-25 revision 30363) [i386-mingw32]
Backport:
[ruby-core:33923]

Description

=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


Files

0001-fix-fiddle-windows-constant.patch (1.48 KB) 0001-fix-fiddle-windows-constant.patch jonforums (Jon Forums), 12/28/2010 12:46 AM
predefs.c (304 Bytes) predefs.c jonforums (Jon Forums), 12/28/2010 11:31 PM
0001-windows-macros-for-dl-n-fiddle.patch (3.53 KB) 0001-windows-macros-for-dl-n-fiddle.patch jonforums (Jon Forums), 12/29/2010 06:39 AM

History

#1

Updated by luislavena (Luis Lavena) about 9 years ago

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

# in ext/dl/extconf.rb
have_header("windows.h")

=end

#2

Updated by jonforums (Jon Forums) about 9 years ago

=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

#3

Updated by tenderlovemaking (Aaron Patterson) about 9 years ago

=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

#4

Updated by Anonymous about 9 years ago

  • 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

#5

Updated by luislavena (Luis Lavena) about 9 years ago

=begin
Thank you Jon and Aaron for applying.

Created backport #4216 for Ruby 1.9.2 maintainer.
=end

#6

Updated by nobu (Nobuyoshi Nakada) about 9 years ago

=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

#7

Updated by tenderlovemaking (Aaron Patterson) about 9 years ago

=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

#8

Updated by jonforums (Jon Forums) about 9 years ago

=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

#9

Updated by luislavena (Luis Lavena) about 9 years ago

=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

#10

Updated by tenderlovemaking (Aaron Patterson) about 9 years ago

=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

#11

Updated by luislavena (Luis Lavena) about 9 years ago

=begin
On Tue, Dec 28, 2010 at 5:00 PM, Aaron Patterson
aaron@tenderlovemaking.com 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

#12

Updated by jonforums (Jon Forums) about 9 years ago

=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

#13

Updated by tenderlovemaking (Aaron Patterson) about 9 years ago

=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

#14

Updated by jonforums (Jon Forums) about 9 years ago

=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

Also available in: Atom PDF