Bug 1541646 - improve support for Skype for Business users
Summary: improve support for Skype for Business users
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: libnice
Version: 27
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Kamil Dudka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-02-03 17:03 UTC by Stefan Becker
Modified: 2018-07-17 14:41 UTC (History)
5 users (show)

Fixed In Version: libnice-0.1.14-7.20180504git34d6044.fc29 libnice-0.1.14-7.20180504git34d6044.fc28 libnice-0.1.14-7.20180504git34d6044.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-07-17 14:41:34 UTC


Attachments (Terms of Use)
git patch to fast-forward to 0.1.14-70-gfb2f1f7 (263.60 KB, patch)
2018-02-03 17:03 UTC, Stefan Becker
no flags Details | Diff
Proposed fix from Phabricator Differential D1717 (698 bytes, patch)
2018-02-27 07:22 UTC, Stefan Becker
no flags Details | Diff
GCC 8.0 -Wcast-function-type errors (10.95 KB, patch)
2018-03-16 20:08 UTC, Stefan Becker
no flags Details | Diff
GCC 8.0 -Wcast-function-type errors (14.50 KB, patch)
2018-05-01 16:41 UTC, Stefan Becker
no flags Details | Diff
Update SPEC file for Fedora 28 (10.86 KB, text/plain)
2018-05-01 16:43 UTC, Stefan Becker
no flags Details
Applying the 2 proposed upstream patches to F28 HEAD (18.40 KB, patch)
2018-05-06 15:54 UTC, Stefan Becker
no flags Details | Diff
Applying the 2 proposed upstream patches to master HEAD (18.06 KB, patch)
2018-05-06 16:14 UTC, Stefan Becker
no flags Details | Diff

Description Stefan Becker 2018-02-03 17:03:38 UTC
Created attachment 1390585 [details]
git patch to fast-forward to 0.1.14-70-gfb2f1f7

This is a continuation from the discussion in bug #1438620 (update libnice to 0.1.14)

After adding a work around in SIPE (see https://sourceforge.net/p/sipe/bugs/337/) to handle duplicate candidates returned by libnice I'm able to call other SfB clients from my Fedora machine **but only if** I fast-forward libnice from 0.1.14 to current git HEAD (0.1.14-70-gfb2f1f7, https://cgit.freedesktop.org/libnice/libnice/commit/?id=fb2f1f77a31baa91968fc81c205f980b6913f403).

Attached is the patch for master. The included patch performs the fast-forward, excluding the patches to modify the unofficial release number (git HEAD is a development version) and the non-existing .gitignore.

I've verified the changes on my F27 machine. So please also apply this change to F27.

Comment 1 Kamil Dudka 2018-02-09 23:32:41 UTC
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.

Comment 2 Stefan Becker 2018-02-27 07:22:45 UTC
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.

Comment 3 Kamil Dudka 2018-02-27 07:58:33 UTC
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?

Comment 4 Stefan Becker 2018-02-27 08:08:26 UTC
(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.

Comment 5 Stefan Becker 2018-02-27 08:11:45 UTC
(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.

Comment 6 Kamil Dudka 2018-02-27 08:41:39 UTC
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.

Comment 7 Stefan Becker 2018-02-27 08:48:13 UTC
(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.

Comment 8 Kamil Dudka 2018-02-27 09:11:38 UTC
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.

Comment 9 Stefan Becker 2018-03-15 19:37:03 UTC
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.

Comment 10 Kamil Dudka 2018-03-16 12:18:40 UTC
(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.

Comment 11 Kamil Dudka 2018-03-16 12:36:37 UTC
(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?

Comment 12 Stefan Becker 2018-03-16 14:35:19 UTC
(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.

Comment 13 Kamil Dudka 2018-03-16 15:16:21 UTC
(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?

Comment 14 Kamil Dudka 2018-03-16 16:25:30 UTC
I have disabled the use of -Werror by default:

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

Comment 15 Stefan Becker 2018-03-16 20:08:45 UTC
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

Comment 16 Kamil Dudka 2018-03-19 08:52:57 UTC
(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

Comment 17 Kamil Dudka 2018-04-05 11:32:53 UTC
(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.

Comment 18 Stefan Becker 2018-05-01 16:41:39 UTC
Created attachment 1429333 [details]
GCC 8.0 -Wcast-function-type errors

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

Comment 19 Stefan Becker 2018-05-01 16:43:04 UTC
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.

Comment 20 Stefan Becker 2018-05-01 16:44:18 UTC
I hope I'll be able to verify the functionality of my Fedora 28 RPM soon.

Comment 21 Kamil Dudka 2018-05-02 12:16:10 UTC
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

Comment 22 Stefan Becker 2018-05-02 12:23:52 UTC
(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.

Comment 23 Kamil Dudka 2018-05-02 13:22:34 UTC
(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).

Comment 24 Stefan Becker 2018-05-06 15:52:11 UTC
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.

Comment 25 Stefan Becker 2018-05-06 15:54:49 UTC
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.

Comment 26 Stefan Becker 2018-05-06 16:14:01 UTC
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.

Comment 27 Kamil Dudka 2018-05-07 09:05:30 UTC
(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

Comment 28 Stefan Becker 2018-05-07 10:06:30 UTC
(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?

Comment 29 Kamil Dudka 2018-05-07 10:14:38 UTC
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 30 Stefan Becker 2018-05-07 10:27:58 UTC
Comment on attachment 1429333 [details]
GCC 8.0 -Wcast-function-type errors

Upstream choose a different solution.

Comment 31 Stefan Becker 2018-05-07 10:31:28 UTC
(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?

Comment 32 Kamil Dudka 2018-05-07 10:41:51 UTC
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.

Comment 34 Fedora Update System 2018-05-07 16:16:38 UTC
libnice-0.1.14-7.20180504git34d6044.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-3216282e36

Comment 35 Fedora Update System 2018-05-10 01:28:21 UTC
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

Comment 36 Fedora Update System 2018-05-13 19:59:55 UTC
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.

Comment 37 Brian J. Murrell 2018-06-27 18:06:50 UTC
This (rpmbuild --)rebuilds with no problem on F27.  Can we get an official build on F27 also?

Comment 38 Kamil Dudka 2018-06-28 10:11:28 UTC
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...

Comment 39 Fedora Update System 2018-06-28 10:49:41 UTC
libnice-0.1.14-7.20180504git34d6044.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-633f12aa29

Comment 40 Fedora Update System 2018-06-28 12:33:49 UTC
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

Comment 41 Fedora Update System 2018-07-17 14:41:34 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.