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

Replace zlib by zlib-ng #2708

Merged
merged 6 commits into from
Sep 8, 2021
Merged

Conversation

dzenanz
Copy link
Member

@dzenanz dzenanz commented Aug 31, 2021

Some PRs in zlib-ng need to be merged, namely, zlib-ng/zlib-ng#1043 and zlib-ng/zlib-ng#1042. Preferably also zlib-ng/zlib-ng#1040 or zlib-ng/zlib-ng#1041. Until then, we will use develop branch in my fork.

@github-actions github-actions bot added area:ThirdParty Issues affecting the ThirdParty module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Aug 31, 2021
@gdevenyi
Copy link
Contributor

gdevenyi commented Sep 1, 2021

Thanks for picking this up @dzenanz. This is well beyond my ability to do right now, but I'll be watching closely for education.

@dzenanz
Copy link
Member Author

dzenanz commented Sep 7, 2021

This works for me. I updated the description with current state. It would be good if someone else tried this to confirm it works as expected.

@dzenanz dzenanz requested review from thewtex and gdevenyi September 7, 2021 20:15
@dzenanz
Copy link
Member Author

dzenanz commented Sep 7, 2021

I haven't tested installation yet.

@dzenanz
Copy link
Member Author

dzenanz commented Sep 7, 2021

I cleaned up the history. Testing the installation is still TODO.

@dzenanz dzenanz changed the title zlib-ng (rebased clone of #2626) for testing Replace zlib by zlib-ng Sep 7, 2021
@dzenanz
Copy link
Member Author

dzenanz commented Sep 7, 2021

Only a minor change was needed to fix installation (the latest commit). Now waiting for upstream to merge my patches.

@thewtex
Copy link
Member

thewtex commented Sep 8, 2021

Since we are using CMake, the Makefile.in files should be removed from the import.

Why is CircleCI failing?

Can we add the name mangling patches to our branch?

@dzenanz
Copy link
Member Author

dzenanz commented Sep 8, 2021

Makefile.in files should be removed from the import

Will do.

Why is CircleCI failing?

/usr/bin/ld: cannot open linker script file /.../ITK/Modules/ThirdParty/ZLIB/src/itkzlib-ng/zlib.map: No such file or directory - I will add this file too. I don't know why only CircleCI needs it.

Can we add the name mangling patches to our branch?

This is currently in my fork. We could go with this while waiting for some variant of the patches to be merged. But that risks the patch to be reject on philosophical/maintenance burden grounds, thus forcing us to keep our own set of patches which will need to be occasionally rebased with probable conflicts..

@thewtex
Copy link
Member

thewtex commented Sep 8, 2021

We could go with this while waiting for some variant of the patches to be merged. But that risks the patch to be reject on philosophical/maintenance burden grounds, thus forcing us to keep our own set of patches which will need to be occasionally rebased with probable conflicts.

Hopefully it will be merged upstream. Or, a different approach could be used upstream, and we will switch to that if it serves the need. But if we need to keep our own set of patches for the name mangling, then that is what we will have to do.

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 well done @dzenanz @gdevenyi !

@gdevenyi
Copy link
Contributor

gdevenyi commented Sep 8, 2021

well done @dzenanz @gdevenyi !

Thanks but @dzenanz definitely did all the hard work. I'm gonna carefully study these changes to learn a bit more CMAKE.

@thewtex
Copy link
Member

thewtex commented Sep 8, 2021

@dzenanz we are getting the error:

CMake Error: File /Users/runner/work/1/s/Modules/ThirdParty/ZLIB/src/itkzlib-ng/zlib_name_mangling.h.empty does not exist.
CMake Error at Modules/ThirdParty/ZLIB/src/itkzlib-ng/CMakeLists.txt:1064 (configure_file):
  configure_file Problem configuring file

@dzenanz
Copy link
Member Author

dzenanz commented Sep 8, 2021

The last force-push takes care of that. ITK.macOS.Python "was cancelled". I will clean up this PR so it can be merged today.

@dzenanz dzenanz marked this pull request as ready for review September 8, 2021 19:38
@thewtex thewtex merged commit 2fab40b into InsightSoftwareConsortium:master Sep 8, 2021
@aaron-bray
Copy link
Contributor

I still seem to be getting this itkzlib-ng not found on the latest master on windows

@aaron-bray
Copy link
Contributor

Hang on, maybe my cmake did not refresh something after pull...

@aaron-bray
Copy link
Contributor

Yeah, note to self, When MSVC asks you to reload, and you click ok, but then it says, sorry can't do that in the process of a build. Close MSVC and reopen it

Nothing to see here
Thanks for your time

@dzenanz dzenanz mentioned this pull request Sep 24, 2021
7 tasks
@dzenanz
Copy link
Member Author

dzenanz commented Sep 24, 2021

Doesn't it give the option to stop the build in order to reload?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants