unbundle minilzo wrt T41974
Open, NormalPublic

Description

although T41974 is already fixed, this is the more appropriate solution

what I tested:

  • linux build
  • static and dynamic linking
  • WITH_LZO=ON and WITH_LZO=OFF

what I did not test:

  • running test suite (is broken here anyway)
  • msvc and osx builds
  • all possible combinations of options

Details

Type
Patch

How should that work on Windows and Mac? We dont have lzo there, I don't see why we need to change this here..

How should that work on Windows and Mac? We dont have lzo there,

Pretty simple. Just bundle the minilzo code only for Windows and Mac then. I can adjust the patch to do that.

I don't see why we need to change this here..

Because you cannot reasonably keep track of all the code you bundle and it's possible vulnerabilities. Distributors cannot and do not rely on a few thousand upstreams to not ship known vulnerable code, for good reason.

Developers should always try to support system versions of any kind of library, because this does not just affect packagers, but regular blender users as well (whether they use a distro-package or not).

This should work with Windows and Mac too. Additionally, there is even an option for linux users to choose which lzo version to use.

Basically looks fine, but some questions:

  • if it's defautl to use system LZO, then perhaps install_deps.sh is to be modified as well?
  • We can consider adding a warning that system lzo not found and that bundled one would be used (that would make previous change unneeded)
  • We can make it a job of FindLZO to return hardcoded locations to minilzo for osx/win
Sergey Sharybin (sergey) triaged this task as "Normal" priority.Sep 30 2014, 11:51 AM

Basically looks fine, but some questions:

  • if it's defautl to use system LZO, then perhaps install_deps.sh is to be modified as well?

Probably. I just never used that script so I missed it.

  • We can consider adding a warning that system lzo not found and that bundled one would be used (that would make previous change unneeded)

I personally dislike build systems that try to be too smart. Even the "if lzo not found, disable the feature" thing is in my opinion not nice, but since the rest of the build system code does that too I just went with it for the sake of consistency.
But actually, I'm more pro fatal warnings, so that it's clear some condition could not be met with the given configuration and hence the user knows the build system will never install a different configuration from what he has specified. That's a pretty important point for distributions, so it may be obvious why I prefer that.

For a practical solution... I'd prefer to print a warning if system LZO was not found which says it will be disabled + that the user can switch to the bundled one.

  • We can make it a job of FindLZO to return hardcoded locations to minilzo for osx/win

What exactly do you mean? I'd prefer to keep the FindLZO module what it is... a generic module, not a blender-specific one.

I don't like the idea of disabling feature if liblzo-dev is not found. It'll be a regression from the builders point of view. Blender is already too complicated to compile, adding just another change which will likely ruin loads folks' days is not good at all.

So imo eitehr we fallback to bundled lzo if system's is not found, or we keep system_lzo defaul off (as we do with, say, bullet).

I don't like the idea of disabling feature if liblzo-dev is not found. It'll be a regression from the builders point of view.

Well, distributors can make the thing fatal by just removing the minilzo folder, so I guess that's not the problem.

I'll fix up the patch shortly.

this patch will fall back to bundled miniLZO if system LZO is not found

Actually... I reworked this a bit for

  • lzo
  • colamd
  • glog/gflags
  • eigen3

Which all have an option to use the system version now. Only lzo and colamd will default to system version, since there seem to be more specific version constraints for gog and eigen3.

From the quick glance i don't mind the patches at all. Couple of questions:

  • What about adding /opt/lib as a hint for the Find*.cmake? Some of blender developers uses it.
  • Ideally SCons also should allow using system libraries

Would you be interested in creating the branch in our repo where you can commit the patches, work further on making scons aware of the system libraries, maybe add options for decoupling more libraries?

P.S. scons might not do find stuff, config variables like BF_LZO_LIBRARIES and such would eb sufficient to make platform maintainers who uses scons happy.

From the quick glance i don't mind the patches at all. Couple of questions:

  • What about adding /opt/lib as a hint for the Find*.cmake? Some of blender developers uses it.

Sure.

  • Ideally SCons also should allow using system libraries

I can't say I am particularly eager to deal with SCons. Why is it still in place? Maintaining two build systems at a time is pretty cumbersome anyway.

Would you be interested in creating the branch in our repo where you can commit the patches, work further on making scons aware of the system libraries, maybe add options for decoupling more libraries?

P.S. scons might not do find stuff, config variables like BF_LZO_LIBRARIES and such would eb sufficient to make platform maintainers who uses scons happy.

Why not, I just hope I don't have to rebase the current patches too often, as in: we should merge what is already fixed.

Obviously there is not much interest here and I don't intend to rebase every 3 months, so please close.

Thomas Dinges (dingto) closed this task as "Archived".Dec 16 2014, 4:25 PM

Using system libraries is handy, its just not very high priority for smaller libraries like this.

Really we should, do the same as OpenJPEG, allow system libraries use but keep extern.

Using system libraries is handy, its just not very high priority for smaller libraries like this.

Really we should, do the same as OpenJPEG, allow system libraries use but keep extern.

I don't understand what this means. None of my recent patches remove bundled libraries.

Maybe my mistake? I just quickly glanced over the the patche linked:

see:

	14 files changed, 58 insertions(+), 10110 deletions(-)
	create mode 100644 build_files/cmake/Modules/FindLZO.cmake
	delete mode 100644 extern/lzo/CMakeLists.txt
	delete mode 100644 extern/lzo/SConscript
	delete mode 100644 extern/lzo/minilzo/COPYING
	delete mode 100644 extern/lzo/minilzo/README.LZO
	delete mode 100644 extern/lzo/minilzo/lzoconf.h
	delete mode 100644 extern/lzo/minilzo/lzodefs.h
	delete mode 100644 extern/lzo/minilzo/minilzo.c
	delete mode 100644 extern/lzo/minilzo/minilzo.h

Committed: 0001-Fix-T41989-support-using-system-lzo.patch rBbb825d02f8570c408f4266dfa4eff53d2d0bf4f6

Will investigate the others later.

Committed: 0004-Allow-using-system-Eigen3.patch rB4b88541d59302d4e65ecc66093a367091538c32d


Still remaining

  • 0002-Allow-using-system-colamd-version.patch - Using arch-linux which has no package for colamd (can make one to test... just noting its not such a common pre-packaged library).
  • 0003-Allow-using-system-glog-gflags-version.patch - @Sergey Sharybin (sergey) wanted to look into this one (apparently it needed some fixes).