| Message ID | 20200724143220.32751-3-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve FreeBSD and macOS jobs in the Cirrus-CI | expand |
On Fri, Jul 24, 2020 at 04:32:19PM +0200, Thomas Huth wrote: > Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, > since -Werror is only enabled for Linux and MinGW builds by default. So > let's enable them here now, too. > For macOS, that unfortunately means that we have to disable the vnc-sasl > feature, since this is marked as deprecated in the macOS headers and thus > generates a lot of deprecation warnings. I wonder if its possible to add #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated" ... #pragma GCC diagnostic pop to silence just one source file ? Regards, Daniel
On Fri, 24 Jul 2020 at 15:32, Thomas Huth <thuth@redhat.com> wrote: > > Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, > since -Werror is only enabled for Linux and MinGW builds by default. So > let's enable them here now, too. > For macOS, that unfortunately means that we have to disable the vnc-sasl > feature, since this is marked as deprecated in the macOS headers and thus > generates a lot of deprecation warnings. For my local OSX builds I use --extra-cflags='-Werror -Wno-error=deprecated-declarations' to work around the SASL thing. thanks -- PMM
On 7/24/20 4:46 PM, Daniel P. Berrangé wrote: > On Fri, Jul 24, 2020 at 04:32:19PM +0200, Thomas Huth wrote: >> Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, >> since -Werror is only enabled for Linux and MinGW builds by default. So >> let's enable them here now, too. >> For macOS, that unfortunately means that we have to disable the vnc-sasl >> feature, since this is marked as deprecated in the macOS headers and thus >> generates a lot of deprecation warnings. > > I wonder if its possible to add > > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored "-Wdeprecated" > > ... > > #pragma GCC diagnostic pop > > to silence just one source file ? 3 years ago Peter said: "The awkward part is that it has to be in force at the point where the deprecated function is used, not where it's declared. So you can't just wrap the #include of the ssl header in pragmas, you'd have to either do it at every callsite or else over the whole .c file." https://www.mail-archive.com/qemu-devel@nongnu.org/msg459264.html I guess we were expecting the distrib to update the pkg. > > > Regards, > Daniel >
On Fri, Jul 24, 2020 at 06:46:23PM +0200, Philippe Mathieu-Daudé wrote: > On 7/24/20 4:46 PM, Daniel P. Berrangé wrote: > > On Fri, Jul 24, 2020 at 04:32:19PM +0200, Thomas Huth wrote: > >> Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, > >> since -Werror is only enabled for Linux and MinGW builds by default. So > >> let's enable them here now, too. > >> For macOS, that unfortunately means that we have to disable the vnc-sasl > >> feature, since this is marked as deprecated in the macOS headers and thus > >> generates a lot of deprecation warnings. > > > > I wonder if its possible to add > > > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored "-Wdeprecated" > > > > ... > > > > #pragma GCC diagnostic pop > > > > to silence just one source file ? > > 3 years ago Peter said: > > "The awkward part is > that it has to be in force at the point where the deprecated > function is used, not where it's declared. So you can't just wrap > the #include of the ssl header in pragmas, you'd have to either > do it at every callsite or else over the whole .c file." Nearly all our sasl code is isolated in ui/vnc-auth-sasl.c, so we can just do pragma push/pop around that entire file. There's then just two remaining cases in ui/vnc.c which are easy enough to deal with, or we can move the calls out of vnc.c into vnc-auth-sasl.c to fully isolate the code > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg459264.html > > I guess we were expecting the distrib to update the pkg. > > > > > > > Regards, > > Daniel > > > Regards, Daniel
On Fri, 24 Jul 2020 at 17:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> I guess we were expecting the distrib to update the pkg.
Apple's view is that you shouldn't be using the sasl header
at all but instead their proprietary crypto library APIs, so
I wouldn't expect them to ever ship something without the
deprecation warnings.
thanks
-- PMM
On Freitag, 24. Juli 2020 18:50:47 CEST Peter Maydell wrote: > On Fri, 24 Jul 2020 at 17:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > I guess we were expecting the distrib to update the pkg. > > Apple's view is that you shouldn't be using the sasl header > at all but instead their proprietary crypto library APIs, so > I wouldn't expect them to ever ship something without the > deprecation warnings. AFAIK Apple's reason for this is similar to no longer providing headers for OpenSSL [1] (or now actually BoringSSL): they cannot guarantee binary compatibility of these libs beyond individual macOS releases (i.e. without breaking old clients) and bad things happened [2] in the past for apps which expected it would. [1] https://lists.apple.com/archives/macnetworkprog/2015/Jun/msg00025.html [2] https://lists.andrew.cmu.edu/pipermail/cyrus-sasl/2007-November/001254.html The common recommendation is: "Ship your macOS app bundled with the preferred version of these libs." Best regards, Christian Schoenebeck
On Fri, 24 Jul 2020 at 10:32, Thomas Huth <thuth@redhat.com> wrote: > > Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, > since -Werror is only enabled for Linux and MinGW builds by default. So > let's enable them here now, too. Reviewed-by: Ed Maste <emaste@freebsd.org> for the FreeBSD change; I'm indifferent on which method is used to address the macos deprecation warnings.
On Sonntag, 26. Juli 2020 18:14:11 CEST Ed Maste wrote: > On Fri, 24 Jul 2020 at 10:32, Thomas Huth <thuth@redhat.com> wrote: > > Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, > > since -Werror is only enabled for Linux and MinGW builds by default. So > > let's enable them here now, too. > > Reviewed-by: Ed Maste <emaste@freebsd.org> > > for the FreeBSD change; I'm indifferent on which method is used to > address the macos deprecation warnings. Like I pointed out on my response to Peter, it is a bit risky to just blindly link against Apple's version of sasl on macOS. Say you compile qemu on a certain macOS version, then you run qemu again after an update of macOS without recompiling qemu, chances are that you get misbehaviours. One solution would be bundling the qemu app along with sasl, so qemu would be forced to use that bundled sasl version instead of whatever Apple's version of sasl is installed on the system. Another appraoch would be checking the system's sasl version on qemu startup by calling either sasl_version() or sasl_version_info() and comparing that with the version qemu was compiled with, and erroring out on doubt. Or obvious last option: simply taking the risk by ignoring this potential issue. The SASL headers are still made available by Apple, which suggests that they don't break SASL 'too often'. Your choice. ;-) Best regards, Christian Schoenebeck
On 24/07/2020 18.49, Daniel P. Berrangé wrote: > On Fri, Jul 24, 2020 at 06:46:23PM +0200, Philippe Mathieu-Daudé wrote: >> On 7/24/20 4:46 PM, Daniel P. Berrangé wrote: >>> On Fri, Jul 24, 2020 at 04:32:19PM +0200, Thomas Huth wrote: >>>> Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, >>>> since -Werror is only enabled for Linux and MinGW builds by default. So >>>> let's enable them here now, too. >>>> For macOS, that unfortunately means that we have to disable the vnc-sasl >>>> feature, since this is marked as deprecated in the macOS headers and thus >>>> generates a lot of deprecation warnings. >>> >>> I wonder if its possible to add >>> >>> #pragma GCC diagnostic push >>> #pragma GCC diagnostic ignored "-Wdeprecated" >>> >>> ... >>> >>> #pragma GCC diagnostic pop >>> >>> to silence just one source file ? >> >> 3 years ago Peter said: >> >> "The awkward part is >> that it has to be in force at the point where the deprecated >> function is used, not where it's declared. So you can't just wrap >> the #include of the ssl header in pragmas, you'd have to either >> do it at every callsite or else over the whole .c file." > > Nearly all our sasl code is isolated in ui/vnc-auth-sasl.c, so we > can just do pragma push/pop around that entire file. > > There's then just two remaining cases in ui/vnc.c which are > easy enough to deal with, or we can move the calls out of vnc.c > into vnc-auth-sasl.c to fully isolate the code Sounds like it could be done, indeed. But I wonder whether we really really want to silence the warnings here? We'd hide the information to the users that sasl is apparently disliked by Apple and might get removed on macOS in the future. Maybe we should rather change the "configure" script to disable sasl by default on macOS unless the user explicitely specified --enable-vnc-sasl ? Thomas
On Mon, 27 Jul 2020 at 06:44, Thomas Huth <thuth@redhat.com> wrote: > Sounds like it could be done, indeed. But I wonder whether we really > really want to silence the warnings here? We'd hide the information to > the users that sasl is apparently disliked by Apple and might get > removed on macOS in the future. > > Maybe we should rather change the "configure" script to disable sasl by > default on macOS unless the user explicitely specified --enable-vnc-sasl ? Does the Homebrew packaging of QEMU depend on and use a Homebrew packaged sasl, or rely on the system sasl ? thanks -- PMM
On 27/07/2020 10.30, Peter Maydell wrote: > On Mon, 27 Jul 2020 at 06:44, Thomas Huth <thuth@redhat.com> wrote: >> Sounds like it could be done, indeed. But I wonder whether we really >> really want to silence the warnings here? We'd hide the information to >> the users that sasl is apparently disliked by Apple and might get >> removed on macOS in the future. >> >> Maybe we should rather change the "configure" script to disable sasl by >> default on macOS unless the user explicitely specified --enable-vnc-sasl ? > > Does the Homebrew packaging of QEMU depend on and use a Homebrew > packaged sasl, or rely on the system sasl ? I don't have a Mac, but looking at https://github.com/Homebrew/homebrew-core/blob/master/Formula/qemu.rb it seems like they don't do anything special with regards to vnc ... so I guess the answer is "they use system sasl" ? Thomas
On Fri, Jul 24, 2020 at 05:50:47PM +0100, Peter Maydell wrote: > On Fri, 24 Jul 2020 at 17:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > I guess we were expecting the distrib to update the pkg. > > Apple's view is that you shouldn't be using the sasl header > at all but instead their proprietary crypto library APIs, so > I wouldn't expect them to ever ship something without the > deprecation warnings. So from pov of our CI, it seems the right answer is to modify the cirrus.yml to install libsasl2 from homebrew: https://formulae.brew.sh/formula-linux/libsasl2 I presume we might need some env variables (CFLAGS/LDFLAGS) to make configure find this version, in preference to the system version. Regards, Daniel
On 26/07/2020 18.14, Ed Maste wrote: > On Fri, 24 Jul 2020 at 10:32, Thomas Huth <thuth@redhat.com> wrote: >> >> Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, >> since -Werror is only enabled for Linux and MinGW builds by default. So >> let's enable them here now, too. > > Reviewed-by: Ed Maste <emaste@freebsd.org> > > for the FreeBSD change; I'm indifferent on which method is used to > address the macos deprecation warnings. Thanks! I think I'll split the FreeBSD change from the macOS changes, since macOS apparently needs some more work... (I'll have a try with Daniel's suggestion to use libsasl2 from Homebrew there...) Thomas
On 27/07/2020 12.57, Daniel P. Berrangé wrote: > On Fri, Jul 24, 2020 at 05:50:47PM +0100, Peter Maydell wrote: >> On Fri, 24 Jul 2020 at 17:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> I guess we were expecting the distrib to update the pkg. >> >> Apple's view is that you shouldn't be using the sasl header >> at all but instead their proprietary crypto library APIs, so >> I wouldn't expect them to ever ship something without the >> deprecation warnings. > > So from pov of our CI, it seems the right answer is to modify the > cirrus.yml to install libsasl2 from homebrew: > > https://formulae.brew.sh/formula-linux/libsasl2 Ok, that one confused me for quite a while, since brew refused to find it in the macOS jobs on Cirrus-CI. The solution: This is not a macOS package, but a Linux package! Homebrew is apparently also available for Linux. There is no libsasl package in homebrew for macOS. So what to do now? I think introducing a libsasl submodule to QEMU just for compiling this code on macOS without warnings is also overkill. And if I got the answers here right, --disable-sasl is also disliked (since this code likely should still be (compile-)tested on macOS). So I think I'll go for the same trick as Peter is using for his tests and use --extra-cflags='-Werror -Wno-error=deprecated-declarations'. Thomas
On Tue, Jul 28, 2020 at 08:43:29AM +0200, Thomas Huth wrote: > On 27/07/2020 12.57, Daniel P. Berrangé wrote: > > On Fri, Jul 24, 2020 at 05:50:47PM +0100, Peter Maydell wrote: > >> On Fri, 24 Jul 2020 at 17:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >>> I guess we were expecting the distrib to update the pkg. > >> > >> Apple's view is that you shouldn't be using the sasl header > >> at all but instead their proprietary crypto library APIs, so > >> I wouldn't expect them to ever ship something without the > >> deprecation warnings. > > > > So from pov of our CI, it seems the right answer is to modify the > > cirrus.yml to install libsasl2 from homebrew: > > > > https://formulae.brew.sh/formula-linux/libsasl2 > > Ok, that one confused me for quite a while, since brew refused to find > it in the macOS jobs on Cirrus-CI. The solution: This is not a macOS > package, but a Linux package! Homebrew is apparently also available for > Linux. There is no libsasl package in homebrew for macOS. Hah, I totally missed that too. > So what to do now? I think introducing a libsasl submodule to QEMU just > for compiling this code on macOS without warnings is also overkill. And > if I got the answers here right, --disable-sasl is also disliked (since > this code likely should still be (compile-)tested on macOS). > So I think I'll go for the same trick as Peter is using for his tests > and use --extra-cflags='-Werror -Wno-error=deprecated-declarations'. Seems like despite the deprecation, people just continue to use the system sasl. I guess the extra-cflags is reasonable for CI, and it gives end users a warning that they're relyin on a feature of macOS that's not considered stable....even if it doesn't appear to have had any actual changes for 10 years. Regards, Daniel
diff --git a/.cirrus.yml b/.cirrus.yml index f287d23c5b..bb25c1723b 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -12,7 +12,7 @@ freebsd_12_task: script: - mkdir build - cd build - - ../configure || { cat config.log; exit 1; } + - ../configure --enable-werror || { cat config.log; exit 1; } - gmake -j8 - gmake V=1 check @@ -24,7 +24,8 @@ macos_task: script: - mkdir build - cd build - - ../configure --python=/usr/local/bin/python3 || { cat config.log; exit 1; } + - ../configure --python=/usr/local/bin/python3 --disable-vnc-sasl + --enable-werror || { cat config.log; exit 1; } - gmake -j$(sysctl -n hw.ncpu) - gmake check @@ -37,6 +38,7 @@ macos_xcode_task: script: - mkdir build - cd build - - ../configure --cc=clang || { cat config.log; exit 1; } + - ../configure --cc=clang --disable-vnc-sasl --enable-werror + || { cat config.log; exit 1; } - gmake -j$(sysctl -n hw.ncpu) - gmake check
Compiler warnings currently go unnoticed in our FreeBSD and macOS builds, since -Werror is only enabled for Linux and MinGW builds by default. So let's enable them here now, too. For macOS, that unfortunately means that we have to disable the vnc-sasl feature, since this is marked as deprecated in the macOS headers and thus generates a lot of deprecation warnings. Signed-off-by: Thomas Huth <thuth@redhat.com> --- .cirrus.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)