unbundle minilzo wrt T41974 #41989
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#41989
Loading…
Reference in New Issue
No description provided.
Delete Branch "%!s(<nil>)"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
although #41974 is already fixed, this is the more appropriate solution
what I tested:
what I did not test:
0001-Fix-#41974-unbundle-minilzo.patch
Changed status to: 'Open'
Added subscriber: @hasufell
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..
Pretty simple. Just bundle the minilzo code only for Windows and Mac then. I can adjust the patch to do that.
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.
0001-Fix-#41989-support-using-system-lzo.patch
Added subscriber: @Sergey
Basically looks fine, but some questions:
Probably. I just never used that script so I missed it.
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.
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
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).
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
0001-Fix-#41989-support-using-system-lzo.patch
Actually... I reworked this a bit for
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
From the quick glance i don't mind the patches at all. Couple of questions:
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.
Sure.
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.
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.
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
.Changed status from 'Archived' to: 'Open'
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:
@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:
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).Removed subscriber: @ideasman42
Added subscriber: @dfelinto
Changed status from 'Confirmed' to: 'Archived'
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.