Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test failures with zlib-ng #274

Open
tomhughes opened this issue Jan 19, 2024 · 7 comments
Open

Test failures with zlib-ng #274

tomhughes opened this issue Jan 19, 2024 · 7 comments

Comments

@tomhughes
Copy link
Contributor

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
Copy link
Member

joto commented Feb 8, 2024

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
Copy link

mmd-osm commented Feb 15, 2024

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

@tomhughes
Copy link
Contributor Author

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
Copy link

zenonp commented Feb 23, 2024

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
Copy link

mmd-osm commented Jul 3, 2024

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

@zenonp
Copy link

zenonp commented Jul 3, 2024

@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
Copy link
Contributor Author

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,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants