unbundle minilzo wrt T41974 #41989

Closed
opened 2014-09-28 17:36:36 +02:00 by Julian Ospald · 29 comments

although #41974 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

0001-Fix-#41974-unbundle-minilzo.patch

although #41974 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 [0001-Fix-#41974-unbundle-minilzo.patch](https://archive.blender.org/developer/F113410/0001-Fix-#41974-unbundle-minilzo.patch)
Author

Changed status to: 'Open'

Changed status to: 'Open'
Author

Added subscriber: @hasufell

Added subscriber: @hasufell

Added subscriber: @ThomasDinges

Added subscriber: @ThomasDinges

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, I don't see why we need to change this here..
Author

In #41989#259324, @ThomasDinges wrote:
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.

In #41989#259324, @ThomasDinges wrote:
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).

> In #41989#259324, @ThomasDinges wrote: > 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. > In #41989#259324, @ThomasDinges wrote: > 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).
Author

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

0001-Fix-#41989-support-using-system-lzo.patch

This should work with Windows and Mac too. Additionally, there is even an option for linux users to choose which lzo version to use. [0001-Fix-#41989-support-using-system-lzo.patch](https://archive.blender.org/developer/F113590/0001-Fix-#41989-support-using-system-lzo.patch)

Added subscriber: @Sergey

Added subscriber: @Sergey

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
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
Author

In #41989#259761, @Sergey wrote:
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.

> In #41989#259761, @Sergey wrote: > 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.

Added subscriber: @ideasman42

Added subscriber: @ideasman42

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. 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).
Author

In #41989#260062, @Sergey wrote:
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.

> In #41989#260062, @Sergey wrote: > 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.
Author

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

0001-Fix-#41989-support-using-system-lzo.patch

this patch will fall back to bundled miniLZO if system LZO is not found [0001-Fix-#41989-support-using-system-lzo.patch](https://archive.blender.org/developer/F114261/0001-Fix-#41989-support-using-system-lzo.patch)
Author

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.

0001-Fix-#41989-support-using-system-lzo.patch

0002-Allow-using-system-colamd-version.patch

0003-Allow-using-system-glog-gflags-version.patch

0004-Allow-using-system-Eigen3.patch

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. [0001-Fix-#41989-support-using-system-lzo.patch](https://archive.blender.org/developer/F114281/0001-Fix-#41989-support-using-system-lzo.patch) [0002-Allow-using-system-colamd-version.patch](https://archive.blender.org/developer/F114282/0002-Allow-using-system-colamd-version.patch) [0003-Allow-using-system-glog-gflags-version.patch](https://archive.blender.org/developer/F114283/0003-Allow-using-system-glog-gflags-version.patch) [0004-Allow-using-system-Eigen3.patch](https://archive.blender.org/developer/F114284/0004-Allow-using-system-Eigen3.patch)

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. - 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.
Author

In #41989#260229, @Sergey wrote:
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.

> In #41989#260229, @Sergey wrote: > 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.
Author

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

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

Changed status from 'Open' to: 'Archived'

Changed status from 'Open' to: 'Archived'

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`.

Changed status from 'Archived' to: 'Open'

Changed status from 'Archived' to: 'Open'
Campbell Barton self-assigned this 2014-12-16 16:39:14 +01:00
Author

In #41989#275322, @ideasman42 wrote:
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.

> In #41989#275322, @ideasman42 wrote: > 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: 0001-Fix-#41974-unbundle-minilzo.patch

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
Maybe my mistake? I just quickly glanced over the the patche linked: [0001-Fix-#41974-unbundle-minilzo.patch](https://archive.blender.org/developer/F113410/0001-Fix-#41974-unbundle-minilzo.patch) 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

@ideasman42, see patches from a comment dated oct 3.

@ideasman42, see patches from a comment dated oct 3.

Committed: 0001-Fix-#41989-support-using-system-lzo.patch bb825d02f8

Will investigate the others later.

Committed: `0001-Fix-#41989-support-using-system-lzo.patch` bb825d02f8 Will investigate the others later.

Committed: 0004-Allow-using-system-Eigen3.patch 4b88541d59


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 wanted to look into this one (apparently it needed some fixes).
Committed: `0004-Allow-using-system-Eigen3.patch` 4b88541d59 ---- 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 wanted to look into this one (apparently it needed some fixes).
Campbell Barton removed their assignment 2016-08-11 06:36:10 +02:00

Removed subscriber: @ideasman42

Removed subscriber: @ideasman42

Added subscriber: @dfelinto

Added subscriber: @dfelinto

Changed status from 'Confirmed' to: 'Archived'

Changed status from 'Confirmed' to: 'Archived'
Dalai Felinto self-assigned this 2019-12-23 18:43:54 +01:00

Hi, thanks for your patch.

We are undergoing a Tracker Curfew where we are automatically closing old patches.

If you think the patch is still relevant please update and re-submit it. For new features make sure there is a clear design from the user level perspective.

Hi, thanks for your patch. We are undergoing a [Tracker Curfew ](https://code.blender.org/?p=3861) where we are automatically closing old patches. If you think the patch is still relevant please update and re-submit it. For new features make sure there is a clear design from the user level perspective.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#41989
No description provided.