ENH: Switch to zlib-ng for modern maintained zlib codebase by gdevenyi · Pull Request #2626 · InsightSoftwareConsortium/ITK

gdevenyi · 2021-06-30T17:13:37Z

See #416

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

kwrobot-v1 · 2021-06-30T17:13:46Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:13:53Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:01Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:09Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:17Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:25Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:33Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:41Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:49Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:14:56Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

kwrobot-v1 · 2021-06-30T17:15:04Z

Errors:

Failed to run the checks: hosting service error: host error: github error: {"message":"Invalid request.\n\nOnly 65535 characters are allowed; 116061 were supplied.","documentation_url":"https://docs.github.com/rest/reference/checks#create-a-check-run"}.

gdevenyi · 2021-06-30T17:15:34Z

Todo:

Modify how the Cmake is called to enable zlib-compat mode
Cleanup the included files to be minimized
Tests

gdevenyi · 2021-06-30T18:34:11Z

I think I've done the update properly. At this point the build fails at:

    152 In file included from /home/gdevenyi/scratch/itk-build/Modules/ThirdParty/ZLIB/src/itk_zlib.h:37,
    153                  from /home/gdevenyi/scratch/ITK/Modules/ThirdParty/NIFTI/src/nifti/znzlib/znzlib.h:60,
    154                  from /home/gdevenyi/scratch/ITK/Modules/ThirdParty/NIFTI/src/nifti/znzlib/znzlib.c:24:
    155 /home/gdevenyi/scratch/ITK/Modules/ThirdParty/ZLIB/src/itkzlib-ng/zlib.h:226:9: error: expected ‘;’ before ‘const’
    156   226 | Z_EXTERN const char * Z_EXPORT zlibVersion(void);
    157       |         ^~~~~~

So this may depend on #1924 as it appears there's some bugs with NIFTI's usage of zlib?

dzenanz

Good progress!

dzenanz · 2021-06-30T19:03:30Z

GDCM also fails to compile:

[2371/5310] Building CXX object Modules/ThirdParty/GDCM/src/gdcm/Source/Common/CMakeFiles/gdcmCommon.dir/gdcmDeflateStream.cxx.o
FAILED: Modules/ThirdParty/GDCM/src/gdcm/Source/Common/CMakeFiles/gdcmCommon.dir/gdcmDeflateStream.cxx.o 
/usr/bin/c++  -IModules/ThirdParty/Expat/src/expat -I/home/vsts/work/1/s/Modules/ThirdParty/Expat/src/expat -IModules/ThirdParty/ZLIB/src -I/home/vsts/work/1/s/Modules/ThirdParty/ZLIB/src -I/home/vsts/work/1/s/Modules/ThirdParty/ZLIB/src/itkzlib-ng -IModules/ThirdParty/GDCM -IModules/ThirdParty/GDCM/src/gdcm/Source/Common -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/DataDictionary -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/Common -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/InformationObjectDefinition -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/MessageExchangeDefinition -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition -IModules/ThirdParty/GDCM/src/gdcm/Testing/Source/Data -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Testing/Source/Data -I/home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Utilities -mtune=generic -march=corei7 -Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch -Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time -Wno-strict-overflow -Wno-deprecated -Wno-invalid-offsetof -Woverloaded-virtual -Wstrict-null-sentinel  -w -fno-ipa-cp-clone -Os -DNDEBUG -fPIC -std=c++14 -MD -MT Modules/ThirdParty/GDCM/src/gdcm/Source/Common/CMakeFiles/gdcmCommon.dir/gdcmDeflateStream.cxx.o -MF Modules/ThirdParty/GDCM/src/gdcm/Source/Common/CMakeFiles/gdcmCommon.dir/gdcmDeflateStream.cxx.o.d -o Modules/ThirdParty/GDCM/src/gdcm/Source/Common/CMakeFiles/gdcmCommon.dir/gdcmDeflateStream.cxx.o -c /home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/Common/gdcmDeflateStream.cxx
In file included from Modules/ThirdParty/ZLIB/src/itk_zlib.h:37:0,
                 from /home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Utilities/gdcm_zlib.h:22,
                 from /home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/Common/zipstreamimpl.h:65,
                 from /home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/Common/gdcmDeflateStream.h:17,
                 from /home/vsts/work/1/s/Modules/ThirdParty/GDCM/src/gdcm/Source/Common/gdcmDeflateStream.cxx:14:
/home/vsts/work/1/s/Modules/ThirdParty/ZLIB/src/itkzlib-ng/zlib.h:226:1: error: 'Z_EXTERN' does not name a type; did you mean 'ZEXTERN'?
 Z_EXTERN const char * Z_EXPORT zlibVersion(void);
 ^~~~~~~~
 ZEXTERN
/home/vsts/work/1/s/Modules/ThirdParty/ZLIB/src/itkzlib-ng/zlib.h:256:1: error: 'Z_EXTERN' does not name a type; did you mean 'ZEXTERN'?
 Z_EXTERN int Z_EXPORT deflate(z_stream *strm, int flush);

Maybe you need to turn on some more backward compatibility options, in addition to set(ZLIB_COMPAT ON)?

gdevenyi · 2021-06-30T20:24:41Z

COMPAT is on, otherwise nothing would work :-) they have a whole new API otherwise

Historically there's a lot of "wrong" ways to use the zlib API that might be disallowed now. I'm gone camping now, I will return to this and Niftilib this weekend.

Code extracted from: https://github.com/zlib-ng/zlib-ng.git at commit c69f78bc5e18a0f6de2dcbd8af863f59a14194f0 (2.0.5).

# By Zlib-ng Upstream * upstream-zlib-ng: zlib-ng 2021-06-25 (c69f78bc)

gdevenyi · 2021-07-02T21:19:39Z

Includes fixed.

Liker fails out now:

Consolidate compiler generated dependencies of target ITKCommon2TestDriver
[ 51%] Linking CXX executable ../../../../bin/ITKCommon2TestDriver
/usr/bin/ld: cannot find -litkzlib
collect2: error: ld returned 1 exit status
Modules/Core/Common/test/CMakeFiles/ITKCommon2TestDriver.dir/build.make:1388: recipe for target 'bin/ITKCommon2TestDriver' failed
make[2]: *** [bin/ITKCommon2TestDriver] Error 1
CMakeFiles/Makefile2:14507: recipe for target 'Modules/Core/Common/test/CMakeFiles/ITKCommon2TestDriver.dir/all' failed
make[1]: *** [Modules/Core/Common/test/CMakeFiles/ITKCommon2TestDriver.dir/all] Error 2
Makefile:165: recipe for target 'all' failed
make: *** [all] Error 2

Need to check if the renaming internally to itkzlib-ng has broken something (either because incomplete or something hard-coded)

gdevenyi · 2021-07-03T02:47:08Z

It looks like the integrated zlib has been running with a bunch of custom patches:
https://gitlab.kitware.com/third-party/zlib/-/compare/master...for%2Fitk-v1.2.11-2017-01-15-cacf7f1d

Will need to figure out which of these changes are needed now, or can be achieved in other ways, and maybe upstream some.

At least some of this was done by @thewtex long ago.

thewtex · 2021-07-03T15:53:54Z

@gdevenyi thanks so much for making this happen! 🏅

We will need to still apply the symbol name mangling on top of upstream. This is required to prevent symbol conflicts when a binary links both itk-zlib-ng and zlib-ng.

The CMake configuration ports likely need to be made on top of upstream, at least in some cases, so files are installed into the ITK module location, etc.

dzenanz · 2021-07-05T14:33:21Z

symbol name mangling

It would make some sense to integrate this into upstream. Other projects could possibly benefit from this (at least VTK, probably some others too).

gdevenyi · 2021-07-05T15:28:11Z

symbol name mangling

It would make some sense to integrate this into upstream. Other projects could possibly benefit from this (at least VTK, probably some others too).

Ok, opened an issue here zlib-ng/zlib-ng#1025, I'll see if I can figure out how to add it for review.

dzenanz · 2021-09-07T20:57:17Z

Closing in favor of #2708.

ENH: Cleanup in preparation for zlib-ng

5988107

github-actions bot added area:ThirdParty Issues affecting the ThirdParty module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Jun 30, 2021

gdevenyi force-pushed the zlib-ng branch 5 times, most recently from cc754b0 to d8f933c Compare June 30, 2021 18:32

dzenanz reviewed Jun 30, 2021

View reviewed changes

gdevenyi and others added 3 commits July 2, 2021 17:15

ENH: Switch to modern maintained zlib-ng fork for zlib

0a2ac02

zlib-ng 2021-06-25 (c69f78bc)

ff85a3b

Code extracted from: https://github.com/zlib-ng/zlib-ng.git at commit c69f78bc5e18a0f6de2dcbd8af863f59a14194f0 (2.0.5).

Merge branch 'upstream-zlib-ng' into zlib-ng

77cfc43

# By Zlib-ng Upstream * upstream-zlib-ng: zlib-ng 2021-06-25 (c69f78bc)

gdevenyi force-pushed the zlib-ng branch from d8f933c to 77cfc43 Compare July 2, 2021 21:16

gdevenyi mentioned this pull request Jul 5, 2021

ENH: Update to NIFTILIB3.0.1 #2630

Merged

7 tasks

dzenanz closed this Sep 7, 2021

dzenanz mentioned this pull request Oct 28, 2021

Integrate modern optimized zlib #416

Closed

gdevenyi deleted the zlib-ng branch December 24, 2021 15:07

Uh oh!

Conversation

gdevenyi commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

kwrobot-v1 bot commented Jun 30, 2021

Uh oh!

gdevenyi commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdevenyi commented Jun 30, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Jun 30, 2021

Uh oh!

gdevenyi commented Jun 30, 2021

Uh oh!

gdevenyi commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdevenyi commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewtex commented Jul 3, 2021

Uh oh!

dzenanz commented Jul 5, 2021

Uh oh!

gdevenyi commented Jul 5, 2021

Uh oh!

dzenanz commented Sep 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gdevenyi commented Jun 30, 2021 •

edited

Loading

gdevenyi commented Jun 30, 2021 •

edited

Loading

gdevenyi commented Jul 2, 2021 •

edited

Loading

gdevenyi commented Jul 3, 2021 •

edited

Loading