Checking compiler warnings as errors on CI

[junaruga]

I am thinking how to check compiler warnings as errors on CI in a better way. It can be possible by adding the -Werror flag to compiler flags. My intention is that this issue ticket manages a feature to check compiler warnings on CI in some way.

My motivation is ..

1. I want to prevent us (me) from unintentionally adding a code that triggers compiler warnings via a pull-request like this issue, Warning: "OPENSSL_FIPS" is not defined, evaluates to 0 [-Wundef] #620.
2. I want to avoid CI failures by a CI case upgrades a compiler to the newer version.

It's difficult to avoid the 2 completely. However, I believe that we can reduce the effort by only adding the -Werror except the *head (head and trufflerruby-head) Ruby on CI.

Proof of concept: Set via MAKEFLAGS

My first proof of concept is to add -Werror via environment variable MAKEFLAGS.

You can check the mkmf.rb and the generated Makefile. Right now every variables are reserved for the specific purposes.

$ ruby ext/openssl/extconf.rb

$ cat Makefile
...
CFLAGS   = $(CCDLFLAGS) $(cflags)  -fPIC  $(ARCH_FLAG)
...
.c.o:
    $(ECHO) compiling $(<)
    $(Q) $(CC) $(INCFLAGS) $(CPPFLAGS) $(CFLAGS) $(COUTFLAG)$@ -c $(CSRCFLAG)$<

Right now I experimented by setting ARCH_FLAG=-Werror. Note the ARCH_FLAG is already used for such as -m64 flag. So, I think we shouldn't use the variable in the real case. Below is the modification. And the CI result is here. You see the macOS Ruby 2.6 and trufflerruby-head cases are failing due to the compiler warnings.

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index becf990..9295001 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -44,7 +44,7 @@ jobs:
 
       # Enable the verbose option in mkmf.rb to print the compiling commands.
       - name: enable mkmf verbose
-        run:  echo "MAKEFLAGS=V=1" >> $GITHUB_ENV
+        run:  echo "MAKEFLAGS=V=1 ARCH_FLAG=-Werror" >> $GITHUB_ENV
         if: runner.os == 'Linux' || runner.os == 'macOS'
 
       - name: compile
@@ -135,7 +135,7 @@ jobs:
         run:  bundle install
 
       - name: enable mkmf verbose
-        run:  echo "MAKEFLAGS=V=1" >> $GITHUB_ENV
+        run:  echo "MAKEFLAGS=V=1 ARCH_FLAG=-Werror" >> $GITHUB_ENV
         if: runner.os == 'Linux' || runner.os == 'macOS'
 
       - name: compile

Right now the mkmf.rb has 3 methods append_cflags, append_cppflags, append_ldflags.

$ grep 'def append_' mkmf.rb
  def append_cppflags(flags, *opts)
  def append_ldflags(flags, *opts)
  def append_library(libs, lib) # :no-doc:
  def append_cflags(flags, *opts)

So, perhaps we can modify the mkmf.rb like this.

$ diff -u mkmf.rb.orig mkmf.rb
--- mkmf.rb.orig	2023-05-19 22:36:37.313241137 +0200
+++ mkmf.rb	2023-05-24 20:12:57.459606769 +0200
@@ -2053,12 +2053,12 @@
 warnflags = #{$warnflags}
 cppflags = #{CONFIG['cppflags']}
 CCDLFLAGS = #{$static ? '' : CONFIG['CCDLFLAGS']}
-CFLAGS   = $(CCDLFLAGS) #$CFLAGS $(ARCH_FLAG)
+CFLAGS   = $(CCDLFLAGS) #$CFLAGS $(ARCH_FLAG) $(EXTCFLAGS)
 INCFLAGS = -I. #$INCFLAGS
 DEFS     = #{CONFIG['DEFS']}
-CPPFLAGS = #{extconf_h}#{$CPPFLAGS}
+CPPFLAGS = #{extconf_h}#{$CPPFLAGS} $(EXTCPPFLAGS)
 CXXFLAGS = $(CCDLFLAGS) #$CXXFLAGS $(ARCH_FLAG)
-ldflags  = #{$LDFLAGS}
+ldflags  = #{$LDFLAGS} $(EXTLDFLAGS)
 dldflags = #{$DLDFLAGS} #{CONFIG['EXTDLDFLAGS']}
 ARCH_FLAG = #{$ARCH_FLAG}
 DLDFLAGS = $(ldflags) $(dldflags) $(ARCH_FLAG)

A challenge of this idea is that MAKEFLAGS is the specification of (GNU?) make. I see this way doesn't work in GitHub Actions Windows cases.

Set via `ruby /path/to/extconf.rb -- with-someting

We can implement the following extra options to the ruby /path/to/extconf.rb. Right now the mkmf.rb has 3 methods append_cflags, append_cppflags, append_ldflags. And the 3 extra options are same with the 3 methods.

• --with-append-cflags="something"
• --with-append-cppflags="something"
• --with-append-ldflags="something"

We can use ruby /path/to/extconf.rb -- --with-append-cflags="-Werror" to append the -Werror to the existing cflags.

There is a similar implementation for the --with-cflags that overrides the CFLAGS.

$ ruby /path/to/extconf.rb -- --with-cflags="something" 

$ bundle exec rake compile -- --with-cflags="something"

Maybe here is the code that sets the value of the --with-cflags.
https://github.com/ruby/ruby/blob/6d976eb5348098a346d82065621e37925acae8b8/lib/mkmf.rb#L2604

[junaruga]

I would close this ticket for now. When I am ready to send the pull-request for this feature, I reopen this ticket.

[junaruga]

Reopened this feature ticket, as I am almost ready to send the pull-request. I am working on the branch, https://github.com/junaruga/openssl/tree/wip/check-compiler-warnings.

[junaruga]

The issue #628 was found related to the -Werror flag.

[junaruga]

Note #603 (comment) .

[rhenium]

1. I want to prevent us (me) from unintentionally adding a code that triggers compiler warnings via a pull-request like this issue, [Warning: "OPENSSL_FIPS" is not defined, evaluates to 0 [-Wundef] #620](https://github.com/ruby/openssl/issues/620).

2. I want to avoid CI failures by a CI case upgrades a compiler to the newer version.

I'm sure the benefits of 1. will outweigh the risk of 2. Trivial C errors slipping through a review have happened many times!

👍

[junaruga]

I am happy to see that you are positive about this feature. :)

I want to consider a logic to turn off of checking the compiler warnings on the GitHub Actions yaml file. It's useful when we see new compiler warnings from new compiler or new OpenSSL or etc, and if we cannot fix the warnings soon, and these warning checks block the CI pipelines.