Bug #1253: Fix MSVC Build Issues

• Status: Closed
• Assignee: usa (Usaku NAKAMURA)
• Target version: 1.9.1
• ruby -v: ruby 1.9.2dev (2009-03-06) [i386-mswin32_90]

[cfis (Charlie Savage)]

=begin
The attached patch fixes a couple of linking bugs with MSVC 2008 against head, plus does a bit of makefile cleanup. I'm happy to split the patch into multiple parts if that would make it easier to apply. The patch includes:

1. LDFLAGS not passed to extconf.rb

For example, in Makefile set LDFLAGS = -libpath:c:/msvc/lib

This path is not passed to exconf.rb, meaning that you can't set the path to third-party extension libraries (zlib/gdbm/readline/iconv/etc).

The problem is Makefile.sub:

+s,@LDFLAGS@,,;t t

This should be

+s,@LDFLAGS@,$(LDFLAGS),;t t

2. -link inconsistently handled.

In VC, -link is a keyword that separates compiler options from linker options. Makefile.sub is consistent on how it is handled.

In some cases:

-link ${LDFLAGS}

In other cases

LDFLAGS = -link ....

My view is that -link should not be included in LDFLAGS, and thus the patch goes with the first pattern.

3. setup.mak out of date

The example compiler/link options specified in setup.mak no longer match what Makefile.sub actually does. The patch fixes this by syncing setup.mak back with Makefile.sub

4. optflags incorrect

OPTFLAGS is set to -O2b2xty- which from the MSDN documentation isn't legal. It should simply be O2 (see http://msdn.microsoft.com/en-us/library/k1ack8f1.aspx).

5. warn flags

For some reason, Makefile.sub turns off all compiler warnings. That seems a bit counter-productive. The patch turns them back on:

!if !defined(WARNFLAGS)
WARNFLAGS = -w3 -wd4996
!endif

-wd4996 turns of deprecation warnings, to avoid all the silly Microsoft warning about non-iso standard replacements for strlen etc.

Let me know if I should split this patch into multiple parts.
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Sun, 8 Mar 2009 08:19:23 +0900,
Charlie Savage wrote in [ruby-core:22725]:

1. LDFLAGS not passed to extconf.rb

For example, in Makefile set LDFLAGS = -libpath:c:/msvc/lib

This path is not passed to exconf.rb, meaning that you can't
set the path to third-party extension libraries
(zlib/gdbm/readline/iconv/etc).

Makefiles for extension libraries use DLDFLAGS instead.

3. setup.mak out of date

The example compiler/link options specified in setup.mak no
longer match what Makefile.sub actually does. The patch
fixes this by syncing setup.mak back with Makefile.sub

4. optflags incorrect

OPTFLAGS is set to -O2b2xty- which from the MSDN
documentation isn't legal. It should simply be O2 (see
http://msdn.microsoft.com/en-us/library/k1ack8f1.aspx).

The page doesn't seem to say it isn't legal. It's just option
crunching.

5. warn flags

For some reason, Makefile.sub turns off all compiler
warnings. That seems a bit counter-productive. The patch
turns them back on:

I'm not sure what you mean. WARNFLAGS is not set by the
default.

!if !defined(WARNFLAGS)
WARNFLAGS = -w3 -wd4996
!endif

-W3?

-wd4996 turns of deprecation warnings, to avoid all the silly
-Microsoft warning about non-iso standard replacements for
-strlen etc.

Possibly, do you mean CRTDEFFLAGS?

Let me know if I should split this patch into multiple parts.

I'd be happy if you would do so next time.

--
Nobu Nakada

=end

[cfis (Charlie Savage)]

=begin

1. Makefiles for extension libraries use DLDFLAGS instead.

But that doesn't work when compiling the conftest programs. For example, say you want to compile the curses extension:

Add this to Makefile:

DLDFLAGS = -libpath:C:\Development\msvc\lib -incremental:no -debug -opt:ref -opt:icf

Then look at mkmf.log for building the extension:

"cl -nologo -Feconftest -I../../.ext/include/i386-mswin32_90 -I../.././../include -I../.././../ext/curses -I../.././../include -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -I. -I./.. -I./../missing -IC:\Development\msvc\include -DLIBRUBY_SO="ruby19.dll" -nologo -MD -RTC1 -Zi -Od -W3 -wd4996 -Od -Zm600 conftest.c ruby19-static.lib tinfo.lib unicows.lib oldnames.lib user32.lib advapi32.lib shell32.lib ws2_32.lib -link -libpath:"." -libpath:"../.." "

Notice that neither LDFLAGS or DLDFLAGS are passed to it? Looking at Makefile.sub:

s,@TRY_LINK@,$$(CC) -Feconftest $$(INCFLAGS) -I$$(hdrdir) $$(CPPFLAGS) $$(CFLAGS) $$(src) $$(LOCAL_LIBS) $$(LIBS) -link $$(LDFLAGS) $$(LIBPATH) $$(XLDFLAGS),;t t

It uses LDFLAGS, not DLDFLAGS. But since LDFLAGS is not set, the conftest does not pass, thus the extension is not built.

5. Warnings.

-W3?

Yes, good catch.

Possibly, do you mean CRTDEFFLAGS?

Ah, didn't know about that. Looks like it does the same thing as -wd4996.

So that leaves the LDFLAGS issue, updating setup.mak, and the -W3 issue. Would you like me to put together separate, updated patches for those?

=end

[usa (Usaku NAKAMURA)]

• Assignee set to usa (Usaku NAKAMURA)

=begin

1. LDFLAGS not passed to extconf.rb

You should use DLDFLAGS, though Nobu has already answered.
However, mkmf.rb is seen not to use DLDFLAGS for conftest like the bug.

2. -link inconsistently handled.

We'll try to think the good solution...

4. optflags incorrect

Rejected. Nobu said the reason.

5. warn flags

We uses compiler's default about warn flags, except for recent VC++'s special warnings.
We know that the warnings are for new programs written for Windows only. Ruby is not such programs.
If you think we are incorrect, please teach us the details.
=end

[cfis (Charlie Savage)]

=begin
Hi Usaka,

Thanks for looking at this. Some more comments inline.

1. However, mkmf.rb is seen not to use DLDFLAGS for conftest like the bug.

And it shouldn't, since its creating executable files, right? DLDFLAGS adds -dll. It still seems to me the simplest solution is to pass in LDFLAGS. Is there a technical issue for not doing that?

2. -link inconsistently handled.

I think what the patch proposes is the correct solution. Remove -link from LDFLAGS. That means users don't have to worry about adding it in if they override LDFLAGS. It also makes the Makefile more consistent and works correctly with conftest (if LDFLAGS is passed in like #1).

4. optflags
   Rejected. Nobu said the reason.

The current setting is -O2b2xty-. The problem with that is -O2 and -Ox are mutually exclusive. From MSDN (http://msdn.microsoft.com/en-us/library/8f8h5cxt.aspx):

-O2 is /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy

And (http://msdn.microsoft.com/en-us/library/59a3b321.aspx):

-Ox is /Og /Oi /Ot /Oy /Ob2 /Os

And last, from MSDN:

"In general, /O2 should be preferred over /Ox and /O1 over /Oxs."

So why not just /O2?

5. warn flags
   We know that the warnings are for new programs written for Windows
   only. Ruby is not such programs.

Why do you think that? Warnings are useful for any program, either with VC2008 and GCC. For example, if you read the mailing lists the postgresql project (I think almost as old as Ruby) spend a lot of time reducing compiler warnings.

Anyway, I think the -W3 level is appropriate for two reasons. First, Visual Studio 2008 sets warnings to -W3 for both production builds and debug builds. Second, according to MSN (http://msdn.microsoft.com/en-us/library/thxezb7y.aspx):

"Level 3 displays all level 2 warnings and all other warnings recommended for production purposes."

So -W3 is what Microsoft recommends. When it is on, it reveals a number of warnings when building Ruby (see #1254). Some look serious, for example:

win32.c(539) : warning C4013: '_RTC_SetErrorFunc' undefined; assuming extern returning int

I'm guessing that is not what is intended.

Thanks for looking at this, and let me know how I can help (happy to make new patches).

=end

[cfis (Charlie Savage)]

=begin
Sorry, I meant to write Usaku not Usaka :( My apologies.
=end

[usa (Usaku NAKAMURA)]

=begin
Hello,

In message "[ruby-core:22768] [Bug #1253] Fix MSVC Build Issues"
on Mar.09,2009 11:08:06, [e-mail] wrote:

4. optflags
   Rejected. Nobu said the reason.

The current setting is -O2b2xty-. The problem with that is -O2 and -Ox are mutually exclusive. From MSDN (http://msdn.microsoft.com/en-us/library/8f8h5cxt.aspx):

They are not mutually exclusive.
-O2b2xty- is:
-Og -Oi -Ot -Oy -Ob2 -Gs -GF -Gy -Ob2 -Og -Oi -Ot -Oy -Ob2 -Os -Ot -Oy-
Yes, this is uselessly too tedious. This is same as:
-Og -Oi -Ot -Oy -Ob2 -Gs -GF -Gy -Os
And there is no exclusive specification.
Of course, we can write this more simple as:
-O2sy-

So why not just /O2?

-Os sometimes makes more faster code.
-Oy sometimes breaks our GC.

5. warn flags
   We know that the warnings are for new programs written for Windows
   only. Ruby is not such programs.

Why do you think that? Warnings are useful for any program, either with VC2008 and GCC. For example, if you read the mailing lists the postgresql project (I think almost as old as Ruby) spend a lot of time reducing compiler warnings.

I only explained our current default warn flags. (especially
why we disabled the two warnings about Microsoft's extensions
outside of the standard)

We can discuss what kind of warn flags is preferable.
But, I'm thinking that -W3 is a little too annoying for us.

Regards,

U.Nakamura [e-mail]

=end

[cfis (Charlie Savage)]

=begin
Hi Usaku,

They are not mutually exclusive.
-Os sometimes makes more faster code.
-Oy sometimes breaks our GC.

Ok. Thanks for the explanation.

I only explained our current default warn flags. (especially
why we disabled the two warnings about Microsoft's extensions
outside of the standard)

Yes, I agree disabling the extensions outside of the standard is a good idea. I think the real question here is this:

Are the warning generated by msvc2008 at -W3 (see #1254) worth fixing? If these are real problems, then the warning should be -W3. If they are not going to be fixed, then maybe the warning level should be lower. But having it turned off like now seems like a bad idea.

Does that makes sense?

Charlie

=end

[cfis (Charlie Savage)]

• File Makefile.sub.patch Makefile.sub.patch added

=begin
Usaku,

To help push this forward, I've attached a new patch. It does two things:

1. Takes -link out of LDFLAGS
2. Passes LDFLAGS to mkmf

I realize there is some hesitancy on point #2, but I don't understand why. This fix makes it possible to compile Ruby 1.9.1 against third party libraries (zlib, dbm, etc) stored in separate directory (because you have to set LIBPATH). It works fine for me with no problems. Am I not understanding something?

Thanks,

Charlie

=end

[nobu (Nobuyoshi Nakada)]

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

=begin
Applied in changeset r22869.
=end

[usa (Usaku NAKAMURA)]

• Status changed from Closed to Open

=begin
#2 and #4 is closed, but others not yet.
=end

[cfis (Charlie Savage)]

=begin
Thanks for applying the LDFLAGS change, that is the most important one.

#2 is still open, the one about applying the -link change. I think that is a better solution than what the makefile does now. But if you choose not to apply it, then you might want to remove the "-link" keyword couple of places it is currently used outside of LDFLAGS.

The other two changes were updating setup.mak and the warnings (-W3). I'll move setup.mak to a new ticket once this one is closed. And on the warnings, I'll make that a separate patch dependent on whether the various compiler warning fix patches I have submitted get accepted or not. So you can consider those two closed for this patch.

Thanks again for taking a look at these issues.

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

=begin
Applied in changeset r22877.
=end