Bug 7245 - zlib is not backwards compatible enough
Summary: zlib is not backwards compatible enough
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.3.1
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.10.0
Assignee: Pierre Ossman
URL:
Keywords: derfian_tester, prosaic
Depends on:
Blocks: 5241 6116
  Show dependency treegraph
 
Reported: 2018-08-31 16:09 CEST by Pierre Ossman
Modified: 2018-09-24 12:43 CEST (History)
1 user (show)

See Also:
Acceptance Criteria:
* No runtime dependency on zlib in any shipped binary * Automatic tests should warn for any dependency on zlib


Attachments

Description Pierre Ossman cendio 2018-08-31 16:09:43 CEST
We need to link zlib statically in many cases (e.g. for Windows). That means we want updated versions of it to get bug fixes and performance improvements.

For Linux we've always linked it dynamically as it is part of LSB core, and it has historically had a very stable API.

Unfortunately it doesn't seem to be true any more. We've now seen at least two cases where the API and ABI has changed. The first was subtle and broke libfontenc in runtime. The second was that libpng depends on symbols added in zlib 1.2.3.4 and zlib 1.2.9 (RHEL 6 has 1.2.3).

We should probably just start linking it statically everywhere.
Comment 3 Pierre Ossman cendio 2018-09-04 09:42:40 CEST
Hmm.... on macOS we use the official SDK so no real risk of depending on something the system doesn't have.

But in order to avoid confusion it's probably better if we are consistent across all our platforms. Let's try to link it statically there as well.
Comment 5 Pierre Ossman cendio 2018-09-04 10:50:31 CEST
Verified that we don't have any dynamic linking of zlib on either Linux, Windows nor macOS.

Also verified that the automatic tests catch this.
Comment 6 Pierre Ossman cendio 2018-09-04 13:07:44 CEST
Some commits accidentally attributed to bug 7246.
Comment 7 Karl Mikaelsson cendio 2018-09-12 09:45:14 CEST
> * Automatic tests should warn for any dependency on zlib> * No runtime dependency on zlib in any shipped binary

✓: autotests are passing
Comment 8 Karl Mikaelsson cendio 2018-09-12 10:22:48 CEST
(In reply to comment #7)
> > * Automatic tests should warn for any dependency on zlib
> 
> ✓

New insights: if autotests does not cover all components that use zlib, this criteria is not met.

> > * No runtime dependency on zlib in any shipped binary
> 
> ✓: autotests are passing
Comment 9 Pierre Ossman cendio 2018-09-12 13:12:17 CEST
It seems we got a performance regression here:

~
[ossman@ossman]$ time gzip -c perf.data > /dev/null

real	0m0.649s
user	0m0.644s
sys	0m0.003s

~
[ossman@ossman]$ time cbrun x86_64 gzip -c perf.data > /dev/null

real	0m1.036s
user	0m0.928s
sys	0m0.114s

This seems to have practical consequences in that Xvnc and vncviewer require more CPU.
Comment 10 Pierre Ossman cendio 2018-09-12 13:56:49 CEST
False alarm. It was the cbrun startup that caused the extra delay.

But I am seeing more CPU usage in Xvnc in these parts. Will have to dig further...
Comment 13 Pierre Ossman cendio 2018-09-17 09:50:19 CEST
(In reply to comment #8)
> (In reply to comment #7)
> > > * Automatic tests should warn for any dependency on zlib
> > 
> > ✓
> 
> New insights: if autotests does not cover all components that use zlib, this
> criteria is not met.
> 

We now have automatic tests for all platforms (added osx64, armhf, win32 and win64).
Comment 14 Karl Mikaelsson cendio 2018-09-24 12:43:37 CEST
(In reply to comment #13)
> We now have automatic tests for all platforms (added osx64, armhf, win32 and
> win64).

Verified that tests are run for osx64, armhf, win32 and win64.

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