integration with the system zlib and libturbojpeg

[ghisvail]

This PR enables optional integration with the system zlib and libturbojpeg libraries, which is a desirable feature for Linux distributions such as Debian or Fedora.

It introduces 2 new options, USE_SYSTEM_ZLIB and USE_SYSTEM_TURBOJPEG to selectively enable compilation with either of the system libraries. They are set to OFF by default in order to keep the old behaviour.

No system libraries:

cmake .. && make
ldd ../bin/dcm2niix

	linux-vdso.so.1 (0x00007ffe2477a000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fbd193c2000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fbd19024000)
	/lib64/ld-linux-x86-64.so.2 (0x000055f9a9efd000)

Use the system zlib:

cmake .. -DUSE_SYSTEM_ZLIB=ON && make
ldd ../bin/dcm2niix

	linux-vdso.so.1 (0x00007ffd3fb1b000)
	libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fa79e481000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fa79e17d000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fa79dddf000)
	/lib64/ld-linux-x86-64.so.2 (0x0000561c3f7fa000)

Use the system zlib and libturbojpeg:

cmake .. -DUSE_SYSTEM_ZLIB=ON -DUSE_SYSTEM_TURBOJPEG=ON && make
ldd ../bin/dcm2niix

	linux-vdso.so.1 (0x00007ffc04176000)
	libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f9ecf9f8000)
	libturbojpeg.so.0 => /usr/lib/x86_64-linux-gnu/libturbojpeg.so.0 (0x00007f9ecf785000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f9ecf481000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f9ecf0e3000)
	/lib64/ld-linux-x86-64.so.2 (0x00005614c50ef000)

[neurolabusc]

Ghislain-
The previous cmake automatically checked if zlib exists and would use it if it did. Your patch only uses this library if the user requests it. It seems the new code is more complex, as it requires the user to know what libraries exist and how to call cmake to enable it. It also will fail if the user runs "cmake .. -DUSE_SYSTEM_ZLIB=ON && make" on a system without zlib installed. With the old code, the advanced user can select between zlib and miniz using the myDisableMiniZ compiler directive. Likewise, the user can switch between nanoJPEG/turboJPEG and neither using the myTurboJPEG and myDisableClassicJPEG directives. These advanced options are described in the readme.md, whereas cmake provides typical users with a sensible, automatic compilation.

My preference would be to have cmake automatically select zlib and turbo-jpeg if they exist, and fall back to the included miniz and nanoJPEG if they do not. That way "cmake" provides the optimal solution and just works.

It looks like this cmake script FindJPEGTURBO.cmake can auto detect the presence of turbo-jpeg in an analogous way to the in-built auto-detection for zlib (using FIND_PACKAGE(ZLIB QUIET) ). Perhaps you could test this out and see if we can have both zlib and turbo-jpeg automatically detected.

[ghisvail]

Ghislain-

Christopher,

Considering the defensive tone of your comments, I realized that I probably did not motivate my PR enough. I will attempt to answer your points one by one in a constructive manner.

The previous cmake automatically checked if zlib exists and would use it if it did.

Correct, and this behaviour is not always desirable for reasons I will explain further in the post.

It seems the new code is more complex.

More flexible.

As far as complexity is concerned, the following pattern:

option(USE_SYSTEM_FOO "" OFF)
if (USE_SYSTEM_FOO)
  find_package(FOO REQUIRED)
  [...do whatever with your targets...]
endif ()

is used absolutely everywhere. I can name you dozens of CMake-based projects using it.

it requires the user to know what libraries exist

The feature of compiling with system libraries is an advanced feature, so yes the user should be aware of what he is doing to enable it. The nice thing with declaring it an option is that it will show up when running ccmake with an explicit description of what the option does.

Actually, do you use ccmake (or any other cmake GUI) at all?

It also will fail if the user runs "cmake .. -DUSE_SYSTEM_ZLIB=ON && make" on a system without zlib installed

And that is intentional. If you request to use the system FOO and FOO cannot be found, the configuration should fail.

With the old code, the advanced user can select between zlib and miniz using the myDisableMiniZ compiler directive.

Which the USE_SYSTEM_ZLIB option correctly handles if I am not mistaken. The latter is also more explicit from a semantic point-of-view.

These advanced options are described in the readme.md, whereas cmake provides typical users with a sensible, automatic compilation.

That's where I think the main point of contention is.

I believe you are underestimating what CMake is and what it can provide. CMake can handle all use cases, i.e. standard build with the vendored code and advanced build with selective linkage with system libraries without leaving the comfort of running cmake + make.

Please tell me how this:

cmake .. -DUSE_SYSTEM_ZLIB=ON && make

is not significantly simpler and explicit than running this:

g++ -O3 -DmyDisableOpenJPEG -I. main_console.cpp nii_dicom.cpp nifti1_io_core.cpp nii_ortho.cpp nii_dicom_batch.cpp jpg_0XC3.cpp ujpeg.cpp -dead_strip -o dcm2niix -lz -DmyDisableMiniZ

for the end-user?

My preference would be to have cmake automatically select zlib and turbo-jpeg if they exist, and fall back to the included miniz and nanoJPEG if they do not.

There are cases where this is not desirable, in particular those involving users with unprivileged rights to their machine. This is quite a common setup in labs using a pool of machine with NFS home directories, and the case in all labs I have been employed in.

Now, say I want to compile dcm2niix and share it within a pipeline to other colleagues. My machine is setup with zlib and turbojpeg but not the others. I cannot share the binary produced on my machine because of the dangling linkage with the system libraries. In this case, I would rather compile without the system libraries for easier deployment, but the non-selective detection would prevent me from doing so without manually hacking the CMake files.

Sure, you might say that I could manually perform the compilation to achieve the desired outcome. I would reply that it is almost 2017 and expecting users to do this manually when CMake is available is just silly.

That way "cmake" provides the optimal solution and just works.

CMake "just works" in all use cases I described in my PR and in this post. Afterall, it is a `Makefile generator, with powerful semantics to handle different code configuration and compilation paths.

It looks like this cmake script FindJPEGTURBO.cmake can auto detect the presence of turbo-jpeg

So does the solution I proposed with pkg-config (also pretty standard).

Perhaps you could test this out and see if we can have both zlib and turbo-jpeg automatically detected.

Did you really read the PR or jumped straight to commenting after looking at the diff?

Did you notice that running this:

cmake .. -DUSE_SYSTEM_ZLIB=ON -DUSE_SYSTEM_TURBOJPEG=ON && make

results in a binary linked with the system turbojpeg. Now perhaps I should have pasted the log of the CMake configuration which would have shown that both ZLIB and TURBOJPEG are automatically detected. AFAIC, no additional find module is required for supporting the system libraries.

Now feel free to merge this PR and subsequently add the FindJPEGTURBO.cmake file you linked. The latter can find turbojpeg in non-standard locations which may or may not be desirable. This is really your call here.

On a closing note, I should apologize for the length of the post and somewhat dry tone towards the end. Amongst, the many projects I have contributed over my years of FLOSS development, this is one of the most frustrating and defensive PR response I have encountered.

[neurolabusc]

I have just merged your suggested code to keep miniz/nanoJPEG as the default. There seem to be 2 compelling reasons:
a.) the executable is more portable: it does not require a static compile or installed libraries.
b.) the executable has been consistently tested and validated with these libraries.

However, you are inferring a tone that was certainly not intended. If I have offended you I apologize. However, I think you can infer my tone by looking at my actions, in each case I think I have been very responsive to suggestions, and have devoted my own time to implementing, validating and merging other peoples suggestions.

1.) WRT issue 8, I adapted Igor's suggestion for zlib as an option instead of miniz. I rejected his/your suggestion for OpenJPEG instead of nanoJPEG on the engineering grounds that OpenJPEG failed to convert classic JPEGS. I wrote the code to support the turbo-JPEG option within 24 hours of you mentioning this library.

2.) WRT to issue 64, I noted that I did not have experience with MAN pages but wrote one and asked for your feedback.

3.) WRT to issue 68 I immediately embraced Jon's code over my own prior solution on engineering grounds. This seems the opposite of defensive behavior.

4.) WRT to the current issue, here I was actually arguing for making your/Igor's preferred zlib/turboJPEG the default rather than my original miniz/nanoJPEG solution. Again, this seems like I was embracing the solution you advocated rather than being defensive. I take your point that relying on zlib/turboJPEG makes the resulting executable dependent on these libraries being present (or requires a static compilation). This was certainly one of the reasons I opted for miniz/nanoJPEG in the first place. I do also want to stress that these are edge cases: classic lossy JPEG for DICOM is rare in my field (lossy and limited to 8 bit precision); my software defaults to pigz for z compression rather than miniz/zlib when possible. Regardless, as I have noted before, I am not very experienced with either debian or cmake, so perhaps what you took as me being defensive was merely me asking for the conventional rationale. Hopefully, the creator of the cmake file and major NeuroDebian contributor @mih can provide his thoughts.

[mih]

FWIW, I just had a look at this and I agree with @ghisvail that this is a solution that works for "tinkerers" as well as packagers. Thanks to @ghisvail for developing the patch and to @neurolabusc for merging it!

As for the frustration that is showing trough the lines of comments I can say this: It may or may not be easily seen, but @ghisvail be assured that @neurolabusc is one of the good ones. We had more than one exchange on "how to do things differently" to make his work a better fit with respect to system integration, and can I attest that the underlying motivation of questions and reconsiderations can safely be assumed to be a desire to fully understand an issue and do the right thing.

I would be saddened if the insufficiencies of email communication or comment threads prevented you from making things better together. Please assume good faith. I owe both of you a beer!

[ghisvail]

First of all, thank you Michael for acknowledging the benefits of my proposed changes. I cannot deny that Christopher has been a very responsive upstream so far (more than the average I deal with daily) and I have absolutely no doubt that his concerns were raised in good faith. My previous comments were solely directed towards this particular PR and the initial answer it received.

On second read however, this was uncalled for:

Amongst, the many projects I have contributed over my years of FLOSS development, this is one of the most frustrating and defensive PR response I have encountered.

And I should apologize to Christopher for it.