Backport #7830

Ruby packages should not build with -Werror when distributed

Added by Ted Kremenek about 1 year ago. Updated 11 months ago.

[ruby-core:52131]
Status:Closed
Priority:Normal
Assignee:Usaku NAKAMURA

Description

I represent the Clang compiler team at Apple, and I've heard some complaints that Clang does not work well for building Ruby. Clearly we care that Clang can compile the Ruby sources.

The complaints seemed to have varied over time, but the most recent one that was reported to me seems captured by the following discussion:

https://github.com/plamoni/SiriProxy/issues/436

Here the problem is that a warning is being promoted to an error via -Werror. While -Werror is a great development tool, it is not a great mechanism to enable for distributing packages as sources. Different compilers, and different versions of the same compiler, may issue different warnings, and using -Werror in source packages makes those packages very brittle to new compiler changes. With Xcode releases, we frequently enable new (or existing) warnings by default that have shown to be quite useful to finding problems. Those new warnings typically are great for development, but when coupled with -Werror and packaged sources (that aren't really changing) they can be disastrous.

I do not actively build Ruby myself, but I request that Ruby software packages should not be distributed with -Werror enabled. This will greatly help users installing Ruby via homebrew or macports, and this likely will be an issue on other platforms besides OS X as well.

disable-werror.patch Magnifier - add --disable-werror new configure option (1.76 KB) Motohiro KOSAKI, 02/12/2013 11:28 AM

disable-werror.patch Magnifier (1.21 KB) Nobuyoshi Nakada, 02/12/2013 04:29 PM

Associated revisions

Revision 40742
Added by Usaku NAKAMURA 11 months ago

merge revision(s) 39214,39221: [Backport #7830]

configure.in: Werror-implicit-function-declaration

* configure.in (warnflags): -Werror-implicit-function-declaration

haven't been used as-is, but always replaced with -Werror= or -W.
* configure.in (warnflags): disable -Werror by default unless
development. [Bug #7830]

History

#1 Updated by Motohiro KOSAKI about 1 year ago

Seems very reasonable request to me.

#2 Updated by Luis Lavena about 1 year ago

kosaki (Motohiro KOSAKI) wrote:

Seems very reasonable request to me.

Kosaki, who should be changing this?

Shall we backport this to 200 and 193 too?

#3 Updated by Motohiro KOSAKI about 1 year ago

Attached RFC patch.

Usa-san, Nagachika-san, I'd like propose to add --disable-werror new configure option. The intention is, trunk developer continue to use -Werror
for finding a mistake as far as earlier and branch maintainers manually disable -Werror when making tar ball. Is this acceptable?

#4 Updated by Nobuyoshi Nakada about 1 year ago

Your patch includes an unintentional change, -Werror-implicit-function-declaration to -Werror=implicit-function-declaration.

#5 Updated by Motohiro KOSAKI about 1 year ago

Your patch includes an unintentional change, -Werror-implicit-function-declaration to -Werror=implicit-function-declaration.

This is intentional. -Werror=implicit-function-declaration and
-Werror-implicit-function-declaration are same meaning and there is no
reason only -Wimplicit-function-declaration keeps exceptional.
Apple made a lot of incompatibility changes in past. We can't believe
they never make the same mistake again.

#6 Updated by Nobuyoshi Nakada about 1 year ago

It's a different story, isn't it?

#7 Updated by Motohiro KOSAKI about 1 year ago

File disable-werror.patch added

It's a different story, isn't it?

No. -Werror-implicit-function-declaration also make compilation error
when compiler make new code analysis. Why do you think so?

#8 Updated by Luis Lavena about 1 year ago

kosaki (Motohiro KOSAKI) wrote:

Attached RFC patch.

Usa-san, Nagachika-san, I'd like propose to add --disable-werror new configure option. The intention is, trunk developer continue to use -Werror
for finding a mistake as far as earlier and branch maintainers manually disable -Werror when making tar ball. Is this acceptable?

Kosaki, I think the option should be the opposite.

By default, -Werror should be disabled and only enabled when --enable-werror is supplied to configure script, your patch just makes this configurable, it doesn't change the default.

#9 Updated by Michal Papis about 1 year ago

The problem is only with clang, maybe we could make the flag default only for it?

#10 Updated by Jon Forums about 1 year ago

It might not be isolated to clang. IIRC, ~1-2 years ago the curb gem failed to build/install with mingw on windows due to -werror settings embedded in RbConfig::CONFIG['warnflags'].

I share Luis' perspective.

#11 Updated by Nobuyoshi Nakada about 1 year ago

kosaki (Motohiro KOSAKI) wrote:

No. -Werror-implicit-function-declaration also make compilation error
when compiler make new code analysis. Why do you think so?

Of course it is same. Unfortunately apple-gcc still doesn't make an
implicit function declaration error by -Werror= option, even if sole
-Werror.

#12 Updated by Nobuyoshi Nakada about 1 year ago

I found that -Werror-implicit-function-declaration has never been used.

#13 Updated by Ted Kremenek about 1 year ago

-Werror-implicit-function-declaration isn't really an issue. It is a standard warning with very clear semantics. If Ruby always builds with that warning promoted to an error that's not likely to be an issue in practice.

The problem is when a new compiler (gcc, clang, or whatever) comes out with a new warning that is enabled by default, or changes some existing warning in a way that causes it to trigger in a way it did not before. That will cause the build to break with -Werror.

#14 Updated by Nobuyoshi Nakada about 1 year ago

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

This issue was solved with changeset r39221.
Ted, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


configure.in: no Werror

  • configure.in (warnflags): disable -Werror by default unless development. [Bug #7830]

#15 Updated by Usaku NAKAMURA about 1 year ago

  • Status changed from Closed to Assigned
  • Assignee set to Tomoyuki Chikanaga

I can accept current state (r39221) to backport 1.9.3.
It seems to match with kosaki-san's proposal at .
nagachika-san, how do you think about this?

#16 Updated by Hans Mackowiak about 1 year ago

i get this with some of my gems:
cc1plus: warning: command line option '-Wdeclaration-after-statement' is valid for C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wimplicit-function-declaration' is valid for C/ObjC but not for C++ [enabled by default]

so do i need to take care about that or can ruby do that for me?

#17 Updated by Nobuyoshi Nakada about 1 year ago

Please file new ticket.

#18 Updated by Tomoyuki Chikanaga about 1 year ago

r39214 and r39221 seems good to me, and I think personally it could be included in 2.0.0 release.
But off course mame-san has final approval for backporting to ruby20_0 before release 2.0.0.

mame-san, how do you think about this? If you decide not to backport them before release 2.0.0, I'll take care about this after release 2.0.0.

#19 Updated by Motohiro KOSAKI about 1 year ago

-Werror-implicit-function-declaration isn't really an issue. It is a standard warning
with very clear semantics. If Ruby always builds with that warning promoted to an error
that's not likely to be an issue in practice.

The problem is when a new compiler (gcc, clang, or whatever) comes out with a new warning
that is enabled by default, or changes some existing warning in a way that causes it to
trigger in a way it did not before. That will cause the build to break with -Werror.

You don't understand current situation correctly. We didn't and never use -Werror. The problem is,
we used -Werror=shorten-64-to-32 and other some specific -Werror= options and Apple changed the
behavior of -Werror=shorten-64-to-32.

So, your comment completely make no sense.

#20 Updated by Ted Kremenek about 1 year ago

kosaki (Motohiro KOSAKI) wrote:

-Werror-implicit-function-declaration isn't really an issue. It is a standard warning
with very clear semantics. If Ruby always builds with that warning promoted to an error
that's not likely to be an issue in practice.

The problem is when a new compiler (gcc, clang, or whatever) comes out with a new warning
that is enabled by default, or changes some existing warning in a way that causes it to
trigger in a way it did not before. That will cause the build to break with -Werror.

You don't understand current situation correctly. We didn't and never use -Werror. The problem is,
we used -Werror=shorten-64-to-32 and other some specific -Werror= options and Apple changed the
behavior of -Werror=shorten-64-to-32.

So, your comment completely make no sense.

I was simply responding to the comment regarding -Werror-implicit-function-declaration, which was posted before my comment.

My main point was that Ruby building with any flag that promotes a warning to an error for source package distribution is a bad idea. No compiler warning should be treated as being stable output between compiler versions and compiler vendors. There are legitimate reasons for this:

(1) Between compiler versions, the behavior of a warning may change. There may be a case that warning that was accidentally not being emitted before (a compiler bug) now gets emitted in a new version. Moreover, some new warnings may get added under an existing -W flag because that's where they belong.

(2) Different compiler vendors support different warnings, or the same warnings with slightly different behavior. What may warn with one compiler may not with another, etc.

For this reason, I believe that Ruby should not build with any of the following flags for source package distribution:

-Werror
-Werror-*
-Werror=*

Hopefully this comment makes more sense, in the broader context in which it was intended.

#21 Updated by Tomoyuki Chikanaga about 1 year ago

  • Tracker changed from Bug to Backport
  • Project changed from ruby-trunk to Backport200

#22 Updated by Tomoyuki Chikanaga about 1 year ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r39564.
Ted, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 39214,39221: [Backport #7830]

configure.in: Werror-implicit-function-declaration

* configure.in (warnflags): -Werror-implicit-function-declaration

haven't been used as-is, but always replaced with -Werror= or -W.
* configure.in (warnflags): disable -Werror by default unless
development. [Bug #7830]

#23 Updated by Tomoyuki Chikanaga about 1 year ago

  • Project changed from Backport200 to Backport93
  • Assignee changed from Tomoyuki Chikanaga to Usaku NAKAMURA

Is it needed in 1.9.3 too?

#24 Updated by Usaku NAKAMURA 11 months ago

  • Status changed from Closed to Assigned

#25 Updated by Usaku NAKAMURA 11 months ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r40742.
Ted, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 39214,39221: [Backport #7830]

configure.in: Werror-implicit-function-declaration

* configure.in (warnflags): -Werror-implicit-function-declaration

haven't been used as-is, but always replaced with -Werror= or -W.
* configure.in (warnflags): disable -Werror by default unless
development. [Bug #7830]

Also available in: Atom PDF