| Message ID | 20191110133525.6069-3-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/3] auth_token_kt: ensure key_type object is initialized | expand |
Am 10.11.19 um 14:35 schrieb Antonio Quartulli: > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > .travis/build-check.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.travis/build-check.sh b/.travis/build-check.sh > index 039a7dcf..250bb454 100755 > --- a/.travis/build-check.sh > +++ b/.travis/build-check.sh > @@ -8,6 +8,7 @@ fi > > if [ "${TRAVIS_OS_NAME}" = "linux" ]; then > export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH:-}" > + export CFLAGS="${CFLAGS} -Werror" > fi > > if [ "${TRAVIS_OS_NAME}" = "osx" ]; then > I assume that you compile tested this, so Acked-By: Arne Schwabe <arne@rfc2549.org>
Hi, On Sun, Nov 10, 2019 at 8:36 AM Antonio Quartulli <a@unstable.cc> wrote: > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > .travis/build-check.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.travis/build-check.sh b/.travis/build-check.sh > index 039a7dcf..250bb454 100755 > --- a/.travis/build-check.sh > +++ b/.travis/build-check.sh > @@ -8,6 +8,7 @@ fi > > if [ "${TRAVIS_OS_NAME}" = "linux" ]; then > export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH:-}" > + export CFLAGS="${CFLAGS} -Werror" > fi > If this is only for linux (and travis), and works with the current code base, probably its fine. But it seems it may also affect mingw builds on travis. I would resist the temptation to add -Werror in general and to Windows cross-build in particular. Not all warnings can be fixed easily. Often the only option is to hide the warning (e.g, by adding a cast) which imo is not always a good strategy. -Werror just forces one's hand to use tricks that hide warnings. Selva <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Nov 10, 2019 at 8:36 AM Antonio Quartulli <a@unstable.cc> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Signed-off-by: Antonio Quartulli <a@unstable.cc><br> ---<br> .travis/build-check.sh | 1 +<br> 1 file changed, 1 insertion(+)<br> <br> diff --git a/.travis/build-check.sh b/.travis/build-check.sh<br> index 039a7dcf..250bb454 100755<br> --- a/.travis/build-check.sh<br> +++ b/.travis/build-check.sh<br> @@ -8,6 +8,7 @@ fi<br> <br> if [ "${TRAVIS_OS_NAME}" = "linux" ]; then<br> export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH:-}"<br> + export CFLAGS="${CFLAGS} -Werror"<br> fi<br></blockquote><div><br></div><div>If this is only for linux (and travis), and works with the current code</div><div>base, probably its fine.</div><div><br></div><div>But it seems it may also affect mingw builds on travis. I would resist the</div><div>temptation to add -Werror in general and to Windows cross-build in particular.</div><div><br></div><div>Not all warnings can be fixed easily. Often the only option is to hide the</div><div>warning (e.g, by adding a cast) which imo is not always a good strategy.</div><div>-Werror just forces one's hand to use tricks that hide warnings.</div><div><br></div><div>Selva</div></div></div>
Hi, On Sun, Nov 10, 2019 at 11:48:16AM -0500, Selva Nair wrote: > But it seems it may also affect mingw builds on travis. I would resist the > temptation to add -Werror in general and to Windows cross-build in > particular. Yeah, we're not intending to. The plan is to have "default builds (no configure options) on Linux with sufficiently current C compilers" warning-free. Which can be achieved with the current code base. > Not all warnings can be fixed easily. Often the only option is to hide the > warning (e.g, by adding a cast) which imo is not always a good strategy. > -Werror just forces one's hand to use tricks that hide warnings. Right. Older systems will warn, oddball systems will warn, non-default build options will warn (and I take one "unused global variable" warning vs. "adding two lines #ifdef/#endif"). gert
Your patch has been applied to the master branch.
commit 1513489be632f6577021e3a0f51799b59c4ace6a
Author: Antonio Quartulli
Date: Sun Nov 10 14:35:25 2019 +0100
travis: compile with -Werror on Linux
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20191110133525.6069-3-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19094.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Forgot to hit send on this, and probably this is only partially relevant now, but here goes. Hi On Sun, Nov 10, 2019 at 12:03 PM Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Sun, Nov 10, 2019 at 11:48:16AM -0500, Selva Nair wrote: > > But it seems it may also affect mingw builds on travis. I would resist > the > > temptation to add -Werror in general and to Windows cross-build in > > particular. > > Yeah, we're not intending to. > But the patch does affect cross-compilation for Windows on linux. Unless restricted to a very small cross-section (say gcc x.y + linux) this is going to cause a lot of grief. Note that every check run by configure that succeeds with warnings will become mysterious error at build time. There may be other unexpected failures too. -Werror is never fit for production in my view. There is a reason why warnings are warnings. Just my 2c. Selva <div dir="ltr"><div>Forgot to hit send on this, and probably this is only partially relevant now, but here goes.</div><div><br></div><div>Hi</div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Nov 10, 2019 at 12:03 PM Gert Doering <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <br> On Sun, Nov 10, 2019 at 11:48:16AM -0500, Selva Nair wrote:<br> > But it seems it may also affect mingw builds on travis. I would resist the<br> > temptation to add -Werror in general and to Windows cross-build in<br> > particular.<br> <br> Yeah, we're not intending to.<br></blockquote><div><br></div><div>But the patch does affect cross-compilation for Windows on linux.</div><div><br></div><div>Unless restricted to a very small cross-section (say gcc x.y + linux) this is going to cause a lot of grief. Note that every check run by configure that succeeds with warnings will become mysterious error at build time. There may be other unexpected failures too. -Werror is never fit for production in my view. There is a reason why warnings are warnings.</div><div><br></div><div>Just my 2c.</div><div><br></div><div>Selva</div></div></div>
Hi, On Sun, Nov 10, 2019 at 05:03:26PM -0500, Selva Nair wrote: > > the > > > temptation to add -Werror in general and to Windows cross-build in > > > particular. > > > > Yeah, we're not intending to. > > But the patch does affect cross-compilation for Windows on linux. That seems to have been an oversight in how to feed Travis what we want - I saw an update yesterday evening. > Unless restricted to a very small cross-section (say gcc x.y + linux) this > is going to cause a lot of grief. Note that every check run by configure > that succeeds with warnings will become mysterious error at build time. > There may be other unexpected failures too. -Werror is never fit for > production in my view. There is a reason why warnings are warnings. "gcc $current + linux" was the plan, and only do it in Travis, not do it in configure for general consumption. gert
Hi, On 11/11/2019 08:23, Gert Doering wrote: >> But the patch does affect cross-compilation for Windows on linux. > > That seems to have been an oversight in how to feed Travis what we want - > I saw an update yesterday evening. > >> Unless restricted to a very small cross-section (say gcc x.y + linux) this >> is going to cause a lot of grief. Note that every check run by configure >> that succeeds with warnings will become mysterious error at build time. >> There may be other unexpected failures too. -Werror is never fit for >> production in my view. There is a reason why warnings are warnings. > > "gcc $current + linux" was the plan, and only do it in Travis, not do > it in configure for general consumption. > Right - this was my mistake while creating this patch. A fix has been sent to the ml yesterday night. That should work as expected. However, note that Travis won't be 100% happy because some compilers don't have the -Wno-stringop-truncation we recently added and configure.ac is failing to detect that.... Cheers,
diff --git a/.travis/build-check.sh b/.travis/build-check.sh index 039a7dcf..250bb454 100755 --- a/.travis/build-check.sh +++ b/.travis/build-check.sh @@ -8,6 +8,7 @@ fi if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH:-}" + export CFLAGS="${CFLAGS} -Werror" fi if [ "${TRAVIS_OS_NAME}" = "osx" ]; then
Signed-off-by: Antonio Quartulli <a@unstable.cc> --- .travis/build-check.sh | 1 + 1 file changed, 1 insertion(+)