Test failures with zlib-ng

tomhughes commented 5 months ago

What version of osmium-tool are you using?

osmium version 1.16.0 libosmium version 2.20.0

What operating system version are you using?

Fedora Rawhide (the upcoming Fedora 40 release)

What did you do exactly?

Built osmium and ran the tests.

What did you expect to happen?

The tests should pass.

What did happen instead?

Tests which compare compressed PBF output failed, namely:

The following tests FAILED:
    328 - formats-f1xp (Failed)
    329 - formats-f1xpd (Failed)
    331 - formats-f1xpm (Failed)
    332 - formats-f1xpdm (Failed)
    335 - formats-f1pp (Failed)
    338 - formats-f1ppd (Failed)
    341 - formats-f1ppc (Failed)
Errors while running CTest

What did you do to try analyzing the problem?

The problem is that Fedora 40 is switching from zlib to zlib-ng for it's zlib implementation and the compressed output is different while producing the same result when decompressed.

This is in reality a broader problem in that the exact form of the compressed data could change at any time - in the past I have seen it change between zlib versions for example.

Options for fixing this would be to unpack the output before comparing it (but then you're not really testing quite the same thing) or to store multiple possible valid outputs for those tests which then requires extensions to the tooling for comparisons to support that.

joto commented 5 months ago

I really don't know how to best solve this.

The tests are there to test the different formats being generated. If we change them to decode those formats, they will be testing a different thing, to the point of at least some of them being useless.
In theory we could have a list of reference files to match against instead of just one reference file. If the test matches any of the files, the test passed. But the tests are already quite complex, because they have to be written in the horrible CMake language. I have no idea how to bring that in there.
If we could figure out from CMake whether we have zlib or zlib-ng, we could run the checks against different reference files. Sure would be ugly, though. But I don't know how to figure that out from CMake anyway.
We can just disable the problemtic tests in default builds and use a special CMake option to enable them.

I am leaning towards the last solution, because it is the simplest. But I am not really happy about it.

mmd-osm commented 4 months ago

Would unit tests also fail when using the zlib-ng-compat package instead?

tomhughes commented 4 months ago

Yes because that is just zlib-ng's zlib compatibility mode that builds libz.so with the traditional API calls - that is in fact exactly what Fedora is using as the zlib replacement.

There is also a libz-ng.so with a new native API which is in the main zlib-ng package.

zenonp commented 4 months ago

I just run into this exact same problem while building osmium-tool-1.16.0-4 (srpm from fedora 40) in mock on almalinux 9.3. However, almalinux AFAIK does not use zlib-ng. The mock build installed zlib-devel 1.2.11-40.el9 from the appstream repo.

Update: I tried again with osmium-tool-1.16.0-1 (srpm from fedora 39) in the exact same mock environment. This time the tests succeeded and the binary rpm was built successfully. Both srpms use the exact same source code, so the difference must be in the .spec file. And quite right,

--- osmium-tool-fc39.spec       2023-09-22 00:00:00.000000000 +0000
+++ osmium-tool-fc40.spec       2024-01-25 00:00:00.000000000 +0000
@@ -5,7 +5,7 @@

 Name:           osmium-tool
 Version:        1.16.0
-Release:        1%{?dist}
+Release:        4%{?dist}
 Summary:        Command line tool for working with OpenStreetMap data

 License:        GPL-3.0-only
@@ -14,8 +14,11 @@
 # Disable tests which break on big endian architectures
 # https://github.com/osmcode/osmium-tool/issues/176
 Patch0:         osmium-tool-bigendian.patch
+# Patch test results for zlib-ng
+# https://github.com/osmcode/osmium-tool/issues/274
+Patch1:         osmium-tool-zlibng.patch

-BuildRequires:  cmake make gcc-c++ pandoc man-db
+BuildRequires:  cmake make gcc-c++ pandoc man-db git-core

 BuildRequires:  catch2-devel >= %{catch_version}
 BuildRequires:  libosmium-devel >= %{libosmium_version}

So there's nothing wrong with the source code or the tests. The problem is the .spec file, which should have conditioned patch1 according to the distro, %if ( 0%{?fedora} && 0%{?fedora} > 39 ) || ( 0%{?rhel} && 0%{?rhel} > 9 ).

mmd-osm commented 4 days ago

So if the issue is really due to spec files (which are maintained elsewhere), we can probably close this one.

zenonp commented 4 days ago

@mmd-osm: Yes, it is an rpm packaging bug, not an osmium-tool bug. But both the problem and the solution have now been documented here and will be useful to the next person facing the same problem.

tomhughes commented 3 days ago

It's not really a packaging bug, it's a "running the tests in any environment that doesn't match upstream" bug.

Incidentally the suggested patch doesn't work (you can't make patch declarations conditional) and is also irrelevant to me as Fedora packager because I'm writing a spec to build for Fedora not a spec to build for arbitrary rpm based distros. Writing a spec for almalinux or RHEL or whatever is a matter for the packager of those distros,

osmcode / osmium-tool