Think so too. Makes more sense to add the flag back. Cheers Lei
On Thu, Sep 8, 2016 at 1:41 PM, Paul Guo <[email protected]> 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 <[email protected]>: > > > 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 <[email protected]>: > > > > > 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 > > > [email protected] > > > > > > > > > > > > > 在 2016年9月6日，上午9:22，Ed Espino <[email protected]> 写道： > > > > > > > > +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 <[email protected]> > > 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 <[email protected]>: > > > >> > > > >>> 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 <[email protected]> > 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* > > > > > > > > >
