You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hawq.apache.org by Paul Guo <pa...@gmail.com> on 2016/09/05 10:05:43 UTC
enforce -Werror (if gcc) in hawq?
-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?
Re: enforce -Werror (if gcc) in hawq?
Posted by Lei Chang <le...@apache.org>.
Think so too. Makes more sense to add the flag back.
Cheers
Lei
On Thu, Sep 8, 2016 at 1:41 PM, Paul Guo <pa...@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 <xu...@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 <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
> > >
> > >
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Roman Shaposhnik <ro...@shaposhnik.org>.
On Wed, Sep 7, 2016 at 11:21 PM, Lei Chang <le...@apache.org> wrote:
> I think consistent behavior is much better to avoid confusions.
>
> different options at different places looks strange. And only people on
> this thread might be able follow this discussion. And maybe someday, they
> will forget this too.
>
> With new contributors/committers added, it is much more easier for them to
> follow simple/default options.
I'm with Lei -- huge +1 for consistency!
Thanks,
Roman.
Re: enforce -Werror (if gcc) in hawq?
Posted by Lei Chang <le...@apache.org>.
I think consistent behavior is much better to avoid confusions.
different options at different places looks strange. And only people on
this thread might be able follow this discussion. And maybe someday, they
will forget this too.
With new contributors/committers added, it is much more easier for them to
follow simple/default options.
Cheers
Lei
On Thu, Sep 8, 2016 at 2:12 PM, Hong Wu <xu...@gmail.com> wrote:
> I still have some reservation and do not recommend that. For the original
> purpose of adding -Werror option, I think following alternative solutions
> are better:
> 1. Try to avoid warnings duringreviewing process.
> 2. Add -Werror in travis CI script. If it fails to compile in travis, it is
> not allowed to check the pull request in.
>
> Above solutions could both be added and not conflict with reasons mentioned
> by Paul. Any comments?
>
> Best
> Hong
>
> 2016-09-08 13:41 GMT+08:00 Paul Guo <pa...@gmail.com>:
>
> > 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 <xu...@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 <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
> > > >
> > > >
> > >
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Hong Wu <xu...@gmail.com>.
I still have some reservation and do not recommend that. For the original
purpose of adding -Werror option, I think following alternative solutions
are better:
1. Try to avoid warnings duringreviewing process.
2. Add -Werror in travis CI script. If it fails to compile in travis, it is
not allowed to check the pull request in.
Above solutions could both be added and not conflict with reasons mentioned
by Paul. Any comments?
Best
Hong
2016-09-08 13:41 GMT+08:00 Paul Guo <pa...@gmail.com>:
> 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 <xu...@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 <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
> > >
> > >
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Paul Guo <pa...@gmail.com>.
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 <xu...@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 <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
> >
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Hong Wu <xu...@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 <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
>
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Zhanwei Wang <wa...@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 <es...@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 <xu...@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 <ml...@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 <pa...@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*
Re: enforce -Werror (if gcc) in hawq?
Posted by Ed Espino <es...@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 <xu...@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 <ml...@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 <pa...@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*
Re: enforce -Werror (if gcc) in hawq?
Posted by Hong Wu <xu...@gmail.com>.
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 <ml...@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 <pa...@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?
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Lili Ma <li...@apache.org>.
+1 on some scan check. We can fix this as the first step.
Thanks
Lili
2016-09-05 21:51 GMT+08:00 Ming Li <ml...@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 <pa...@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?
> >
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Ming Li <ml...@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 <pa...@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?
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Hong Wu <xu...@gmail.com>.
-Werror is an option used to make sure that compiling warnings are
potentially bugs or errors. However, there is some trade-off between the
usage of C code(from standard's view) and the specific implementation of
gcc.
In my opinion, C grammer is not that complex so we'd better to open this
option. You should try to open the flag and see what is going on. Just do
it.
Best
Hong
2016-09-05 18:05 GMT+08:00 Paul Guo <pa...@gmail.com>:
> -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?
>
Re: enforce -Werror (if gcc) in hawq?
Posted by Lei Chang <ch...@gmail.com>.
In the past, it is enabled by default. Looks like it is a regression during oss and build refactoring process.
Cheers
Lei
On Mon, Sep 5, 2016 at 6:05 PM +0800, "Paul Guo" <pa...@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?