hawq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lei Chang <lei_ch...@apache.org>
Subject Re: enforce -Werror (if gcc) in hawq?
Date Thu, 08 Sep 2016 06:11:04 GMT
Think so too. Makes more sense to add the flag back.

Cheers
Lei


On Thu, Sep 8, 2016 at 1:41 PM, Paul Guo <paulguo@gmail.com> wrote:

> For HAWQ, it is an open source project so I think from this respective, we
> should
> try to make all contributors have  the same development/test experience,
> else
> main contributors (e.g. committer) will have to waste time to fix those
> errors introduced
> by some contributors who know nothing about the context. Making the
> build/test
> less complex is our target. This is my answer to Hong and concern 1) of
> Zhanwei
>
> Back to the -Wall and -Werror details. To my best of knowledge,
> 1) gcc has been adjusting (probably mainly adding) the warning cases.
> 2) -Wall actually enables just part of warnings and the set of warning
> could
>     vary in various gcc versions.
>
> But I still think it should be enabled by default. The reasons are:
>
> 1) We actually just mainly tested on centos6.x by now. We actually did not
> test on
>     mac and centos7 that much. So "officially" we do not support that many
> os and
>     the fix will be not that tedious.
>
> 2) In the future if we want to have more os support, "gcc -Werror" is just
> one small
>     step of the whole work. I do not think this introduces much more work.
>
> 3) Although different gcc versions have different -Wall definitions, I
> think the fixes
>     for various gcc versions should be quite mutually tolerated.
>
> 4) Even above solutions do not work finally, we could specifies common
> warnings
>    via -Werror=, e.g. -Werror=-Wunused-variable,...
>
>
>
> 2016-09-06 10:31 GMT+08:00 Hong Wu <xunzhangthu@gmail.com>:
>
> > I strongly agree with ZhanWei's opinion, I think it is much more flexible
> > to honor environment variable to do that. Take CMake system for example,
> it
> > is usual to honor "-DCMAKE_XXX" when doing configuration.
> >
> > As ZhanWei's recommendation, I think we should try it ourselves to make
> > sure good coding style and code quality. For customers and contributors
> > from community, it is better not doing that by default.
> >
> > Thanks
> > Hong
> >
> > 2016-09-06 9:52 GMT+08:00 Zhanwei Wang <wangzw@apache.org>:
> >
> > > Hi Hong
> > >
> > > I removed -Werror flag when we push HAWQ to open source. In Pivotal we
> > > build HAWQ on specific OS with specific GCC and dependent libraries.
> > > -Werror flag worked fine since we fix all warnings. But for a open
> source
> > > project. It is impossible to make users and contributors to build HAWQ
> on
> > > specific OS and compiler just like what we did before. We cannot say we
> > > just support HAWQ on Centos 6 with GCC 4.4.2.
> > >
> > > So if we enforce -Werror flag, I guess the build will fail on many
> > > environments.
> > >
> > > I propose three solutions and let’s discuss which is better.
> > >
> > > 1) Do not enforce -Werror flag but add it to our test (concourse and
> > > Travis) like this.
> > > ./configure —prefix=/path/to/install CFLAGS=“-Werror”
> > >
> > > By this way we can enforce -Werror flag on our tested environment.
> > >
> > >
> > > 2) Only enforce -Werror flag on development build, remove it on release
> > > build.
> > >
> > >
> > > 3) Enforce -Werror flag and we setup more test environments(different
> > > versions of CentOS Ubuntu SUSE with default compiler and latest GCC and
> > > MacOS with default compiler and latest clang)
> > >
> > >
> > > Any comments?
> > >
> > >
> > > Best Regards
> > >
> > > Zhanwei Wang
> > > wangzw@apache.org
> > >
> > >
> > >
> > > > 在 2016年9月6日,上午9:22,Ed Espino <espino@apache.org>
写道:
> > > >
> > > > +1
> > > >
> > > > Have we considered setting up separate public Concourse pipelines to
> > try
> > > > the various build scenarios.
> > > >
> > > > -=e
> > > >
> > > > On Tue, Sep 6, 2016 at 12:58 AM, Hong Wu <xunzhangthu@gmail.com>
> > wrote:
> > > >
> > > >> Ming's comment makes sense, but I think it is another thread. I have
> > > >> already tried those[1] but there are some further works need to
> do[2].
> > > >>
> > > >> [1]
> > > >> - clang analysis scan report: I uploaded the result of not that
> fresh
> > > HAWQ
> > > >> in my personal link, please check the report out here
> > > >> <http://xunzhangthu.org/tmp/hawq_check> if you are interested.
> > > >> - coverity scan: the latest reported is here
> > > >> <https://scan.coverity.com/projects/apache-incubator-hawq>.
If you
> > > want to
> > > >> see the defects in detail, you need to submit a permission request.
> > > >>
> > > >> [2]
> > > >> - Make clang analysis scan reported generating periodically and
> > > publishing
> > > >> to the public automatically. I suggest we donate a domain such as
> > > hawq.io
> > > >> and
> > > >> a host for it, also for automatically publishing the report, we need
> > to
> > > >> write a web service to reply the requests and transmitting html
> data.
> > > >>
> > > >> - Travis CI script have already integrated the coverity scan service
> > > using
> > > >> github webhook. we need to create a coverity_scan branch for hawq
> and
> > > then
> > > >> modify the .travis.yml file. I have done that under Redhat
> > environment.
> > > >> While the only environment Travis server supports for Linux is
> > > >> Ubuntu/Debian. Although hawq could be built under Ubuntu, it needs
> > extra
> > > >> effort to extend the .travis.yml script to support that. For osx
> > > >> environment, I am not sure what the problem is, the issue is that
> the
> > > >> report could not be sent to the coverity scan server automatically.
> > > >>
> > > >> ps: I think Chunlin <https://github.com/wcl14> is starting working
> on
> > > the
> > > >> defects generated by coverity.
> > > >>
> > > >>
> > > >> Back to main thread mentioned by Paul, I think we should just try
to
> > > open
> > > >> the flag and discuss errors after opening -Werror.
> > > >>
> > > >> Best
> > > >> Hong
> > > >>
> > > >>
> > > >> 2016-09-05 21:51 GMT+08:00 Ming Li <mli@pivotal.io>:
> > > >>
> > > >>> Good suggestion.
> > > >>>
> > > >>> However, IMHO, we may need to firstly enable coverity scan check
or
> > > clang
> > > >>> analysis scan. Also we should make the output of these check on
a
> > > public
> > > >>> server so that all contributor can access them.
> > > >>>
> > > >>> On Mon, Sep 5, 2016 at 6:05 PM, Paul Guo <paulguo@gmail.com>
> wrote:
> > > >>>
> > > >>>> -Werror
> > > >>>> Make all warnings into errors.
> > > >>>> I've seen many cases (not just hawq) before that ignoring
gcc
> > warning
> > > >>> leads
> > > >>>> to bugs. I'm wondering we should add the option for the gcc
case.
> > > Given
> > > >>>> there may be a lot of warnings when building the common postgres
> > code
> > > >> in
> > > >>>> hawq, we could at least enforce it in our own code at first
> > > >>>> (src/backend/cdb, src/backend/resourcemanager, src/test/feature,
> > other
> > > >>>> directories?)? Any suggestion?
> > > >>>>
> > > >>>
> > > >>
> > > >
> > > >
> > > >
> > > > --
> > > > *Ed Espino*
> > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message