Bug 1541646

I have pushed it to Fedora rawhide for now.  While the build succeeds on f27, there were a lot of compilation errors caused by casts between incompatible function types on f28.  I have turned them over into warnings again by the following commit to make the build succeed:

https://src.fedoraproject.org/cgit/rpms/libnice.git/commit/?id=6eb05714

But the code could hardly work correctly.   There are some functions that take multiple parameters passed to callbacks that expect a function with exactly one parameter.  Could you please have a look at those errors/warnings?

I have also (re)enabled the upstream test-suite in %check.  It nearly does not work in Koji because multicast traffic is blocked there but it could be (more) useful in case the package is built locally.

Created attachment 1401157 [details]
Proposed fix from Phabricator Differential D1717

Latest update:

* SIPE 1.23.1 has been released which has a workaround for libnice duplicate candidates issue. I.e. proposed fix from Phabcicator Differential D1949 (https://phabricator.freedesktop.org/D1949) can be included, but is not mandatory
* application sharing requires attached proposed fix from Phabricator Differential D1727 (https://phabricator.freedesktop.org/D1727)

With this patch level I can now use all SIPE functionality with my Skype for Business cloud based account on F27.

Thank you for sharing the patch!

I am not able to find it at https://anongit.freedesktop.org/git/libnice/libnice.git -- is there any plan to merge it?

Have you had time to look at the -Wcast-function-type compilation errors/warnings?

(In reply to Kamil Dudka from comment #3)
> is there any plan to merge it?

Phabricator is their code review tool, so yes that patch isn't merged yet. I'm also wondering what it will take to get those two changes merged...

> Have you had time to look at the -Wcast-function-type compilation
> errors/warnings?

No not yet. If those are related to GObject then please see bug #1546548.

(In reply to Stefan Becker from comment #4)
> (In reply to Kamil Dudka from comment #3)
> 
> > Have you had time to look at the -Wcast-function-type compilation
> > errors/warnings?
> 
> No not yet. If those are related to GObject then please see bug #1546548.

A quick check through the source code reveals that libnice is GObject based. So that bug definitely applies and might fix your build issue.

Thanks for the reference!  As I understand it, the fix applied on glib seems to be equivalent to the -Wno-error=cast-function-type compiler flag, which I added to libnice.spec to make it build.  It does not fix the type incompatibility but only suppresses those warnings/errors.

(In reply to Kamil Dudka from comment #6)
> Thanks for the reference!  As I understand it, the fix applied on glib seems
> to be equivalent to the -Wno-error=cast-function-type compiler flag, which I
> added to libnice.spec to make it build.  It does not fix the type
> incompatibility but only suppresses those warnings/errors.

The problem with glib2 headers can't be fixed in client source code. So bug #1546548 is the only way forward there.

Once that bug is closed you should remove the suppression again to make sure there are not other things in libnice itself that *DO* need to be fixed.

Yes, my change just turned those errors back to warnings.  So we are in the same situation as on Fedora 27.  Moreover it is enabled for Fedora 28 only, so it will force us to review it during the Fedora 29 rebuild.

glib2 2.56.0 has been built for F28/rawhide. I verified with rawhide that libnice now compiles without the workaround. I would suggest to remove it from F28 after the alpha release.

(In reply to Stefan Becker from comment #9)
> glib2 2.56.0 has been built for F28/rawhide. I verified with rawhide that
> libnice now compiles without the workaround.

The workaround is already disabled if you build libnice for rawhide.

> I would suggest to remove it from F28 after the alpha release.

I am fine with removing it as soon as we are able to build libnice without it against f28 build root.  Note that the new glib2 needs to pass through Bodhi first.

(In reply to Kamil Dudka from comment #10)
> (In reply to Stefan Becker from comment #9)
> > glib2 2.56.0 has been built for F28/rawhide. I verified with rawhide that
> > libnice now compiles without the workaround.
> 
> The workaround is already disabled if you build libnice for rawhide.

Actually, I cannot confirm that libnice builds successfully in rawhide with glib2-2.56.0-1.fc29 installed.  Please have a look at the following scratch build:

https://koji.fedoraproject.org/koji/taskinfo?taskID=25740929

How exactly did you try to build it?

(In reply to Kamil Dudka from comment #11)
> https://koji.fedoraproject.org/koji/taskinfo?taskID=25740929

Those are different errors than the ones I reported in bug #1546548. Please check your build log before you applied your workaround. You should notice different errors there.

> How exactly did you try to build it?

I did a mock -r fedora-rawhide-x86_64 with the SRPM that I've already used to build my version for F27 from. I.e. libnice-0.1.14 plus the two patches I have attached to this bug report.

(In reply to Stefan Becker from comment #12)
> I did a mock -r fedora-rawhide-x86_64 with the SRPM that I've already used
> to build my version for F27 from. I.e. libnice-0.1.14 plus the two patches I
> have attached to this bug report.

Thank you for clarifying it!

Then it is just a consequence of having LIBNICE_RELEASE="no" in configure.ac because "no" implies -Werror.  So with your SRPM you see all those issues as warnings only.

We can easily flip it back to LIBNICE_RELEASE="yes" so that all warnings are ignored again (instead of ignoring selected warnings only).  Is this what you ask for?

I have disabled the use of -Werror by default:

https://src.fedoraproject.org/cgit/rpms/libnice.git/commit/?id=cb05dbf2

Created attachment 1408961 [details]
GCC 8.0 -Wcast-function-type errors

(In reply to Kamil Dudka from comment #13)
> (In reply to Stefan Becker from comment #12)
> Then it is just a consequence of having LIBNICE_RELEASE="no" in configure.ac
> because "no" implies -Werror.

Yes, my source tree has LIBNICE_RELEASE="yes" which sets enable_compile_warnings to "yes".

(In reply to Kamil Dudka from comment #14)
> I have disabled the use of -Werror by default:

Not a good idea, as upstream won't be notified. I've now added "--enable-compile-warnings=error" to configure and "V=1" to make in my SRPM to investigate the errors.

I've created a proposed fix upstream https://phabricator.freedesktop.org/T7942

(In reply to Stefan Becker from comment #15)
> Not a good idea, as upstream won't be notified.

I fail to see how using or not using -Werror in downstream builds affects whether upstream is notified or not.  You can build without -Werror and and send patches upstream for the warnings that are reported during the build.

Problem of -Werror is that the build stops prematurely if a warning is detected.  Moreover, if you build in parallel, it is undefined how many warnings/errors are reported before the build stops.

There are better ways to capture GCC warnings from Fedora packages.  Did you try csmock?

$ sudo dnf install csmock
$ koji download-build --arch=src libnice-0.1.14-5.20171128gitfb2f1f7.fc29
$ csmock -r fedora-rawhide-x86_64 -t gcc libnice-0.1.14-5.20171128gitfb2f1f7.fc29.src.rpm

(In reply to Stefan Becker from comment #2)
> * SIPE 1.23.1 has been released which has a workaround for libnice duplicate
> candidates issue. I.e. proposed fix from Phabcicator Differential D1949
> (https://phabricator.freedesktop.org/D1949) can be included, but is not
> mandatory

This patch seems to be merged upstream:

https://github.com/libnice/libnice/commit/54fb0342

> * application sharing requires attached proposed fix from Phabricator
> Differential D1727 (https://phabricator.freedesktop.org/D1727)

Still waiting for this one to be merged.

Created attachment 1429333 [details]
GCC 8.0 -Wcast-function-type errors

Updated patch to cover "make check" build failures too.

Created attachment 1429345 [details]
Update SPEC file for Fedora 28

Changes on top of Fedora 28 package to add 2 additional patches.

I had to disable the tests, because on my machine test-gstreamer crashes.

I hope I'll be able to verify the functionality of my Fedora 28 RPM soon.

Stefan, could you please rebase your changes on top of the current master branch?

Should those changes fix some functional bug or a build failure?

The packages seems to build fine in Koji:

https://koji.fedoraproject.org/koji/taskinfo?taskID=26725367

(In reply to Kamil Dudka from comment #21)
> Stefan, could you please rebase your changes on top of the current master
> branch?

The GCC 8.0 change is against master, I do not own the other change:

$ git lg
* b8b5096 (HEAD -> topic-gcc8.0) Fix GCC 8.0 -Wcast-function-type compilation ...
* 1e528ea component: accept TURN in nice_component_verify_remote_candidate()...
* 922ee4e (origin/master, origin/HEAD, master) agent: don't require "reliable"...

> Should those changes fix some functional bug or a build failure?

1e528ea is a functionality fix, see my comment on: https://phabricator.freedesktop.org/D1727

> The packages seems to build fine in Koji:

I removed the F28 flagging from my SPEC, i.e. -Werror is enabled and hence the GCC 8.0 fixes are required.

(In reply to Stefan Becker from comment #22)
> The GCC 8.0 change is against master, I do not own the other change:

I meant the attached spec file.  It conflicts with the current master branch in dist-git.

> 1e528ea is a functionality fix, see my comment on:
> https://phabricator.freedesktop.org/D1727

I know about that one (see comment #17).  Unfortunately, upstream does not seem to be very active in merging those patches.  I think we can temporarily adopt the patch downstream (after you confirm that it works as expected).

I just tried a local mock build from vanilla F28 HEAD () and one test fails, causing the whole package build to fail:

...
PASS: test-socket-is-based-on
PASS: test-priority
PASS: test-fullmode
**
libnice-tests:ERROR:test-different-number-streams.c:137:main: assertion failed: (nice_agent_gather_candidates (lagent, ls_id) == TRUE)
/bin/sh: line 5:  9909 Aborted                 (core dumped) GST_PLUGIN_PATH=:../gst ${dir}$tst
FAIL: test-different-number-streams
PASS: test-restart
...

I guess that test is still unstable as it must have passed for the koji build.

Created attachment 1432400 [details]
Applying the 2 proposed upstream patches to F28 HEAD

Replacing the SPEC with git format-patch output after applying the changes to F28 HEAD.

Created attachment 1432401 [details]
Applying the 2 proposed upstream patches to master HEAD

Cherry-picked from F28 to master and fixed the merge errors.

Please note that I only get the package build passed locally after applying a local change that disables "test-different-number-streams". This change is not included in this patch set.

(In reply to Stefan Becker from comment #24)
> I guess that test is still unstable as it must have passed for the koji build.

The test passes in my local mock build, too.  It was contributed last fall:

https://phabricator.freedesktop.org/D1880

I guess it would make sense to notify its author(s).

(In reply to Stefan Becker from comment #26)
> Cherry-picked from F28 to master and fixed the merge errors.

It is not very common to cherry-pick something something from a stable branch to master.  It usually works the other way around because the development primarily happens in the master branch.  Nevertheless, in this case I would prefer to keep the f28 branch identical with the master branch.

As for the GCC 8.0 patch, it does not seem to be accepted by libnice upstream.  They have pushed the following patch instead:

https://github.com/libnice/libnice/commit/e6217f8e

(In reply to Kamil Dudka from comment #27)
> As for the GCC 8.0 patch, it does not seem to be accepted by libnice
> upstream.  They have pushed the following patch instead:

Thanks. I closed my upstream task. Can you please move to libnice HEAD?

Yes, I see no immediate reason why it would cause a problem.  Do we still need https://phabricator.freedesktop.org/D1727 on top of that for Business Skype to work correctly?

Comment on attachment 1429333 [details]
GCC 8.0 -Wcast-function-type errors

Upstream choose a different solution.

(In reply to Kamil Dudka from comment #29)
> Do we still
> need https://phabricator.freedesktop.org/D1727 on top of that for Business
> Skype to work correctly?

It was necessary when I tried application sharing last time. In this mornings meeting application sharing did not work. But I don't have time and resources at the moment for another debugging session with the media developer.

At least audio & video calls for SfB accounts will benefit from updated libnice @ master HEAD. Will you also update the F28 package?

OK.  I will update libnice to latest upstream and apply the mentioned patch on top of that, in both rawhide and Fedora 28.  We can revert it back in case it causes any problems.

downstream commits:

https://src.fedoraproject.org/cgit/rpms/libnice.git/commit/?id=9cda0800
https://src.fedoraproject.org/cgit/rpms/libnice.git/commit/?id=a04a646f

libnice-0.1.14-7.20180504git34d6044.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-3216282e36

libnice-0.1.14-7.20180504git34d6044.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-3216282e36

libnice-0.1.14-7.20180504git34d6044.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

This (rpmbuild --)rebuilds with no problem on F27.  Can we get an official build on F27 also?

It is good that it builds with no problems :-)  I would more appreciate feedback on whether the built packages actually work as expected because I am not able to try it out myself.

Anyway, I am not against submitting the update for f27, too.  Let's see whether users will be able to test it...

libnice-0.1.14-7.20180504git34d6044.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-633f12aa29

libnice-0.1.14-7.20180504git34d6044.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-633f12aa29

libnice-0.1.14-7.20180504git34d6044.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.